[PATCH] credential-cache: interpret an ECONNRESET as on EOF

[Ramsay Jones]

Since commit 612c49e94d ("credential-cache: add tests for XDG
functionality", 17-03-2017), the cygwin build has been failing all the
new tests added by that commit. In particular, the 'git credential-cache
exit' command, as part of the test cleanup code, has been die-ing with
the message:

fatal: read error from cache daemon: Connection reset by peer

As this git command is part of an && chain in a 'test_when_finished'
call, the remaining test cleanup is not happening, so practically all
remaining tests fail due to the unexpected presence of various socket
files and directories.

A simple means of getting the tests to pass, is to simply ignore the
failure of 'git credential-cache exit' command and make sure all test
cleanup is done. For example, the diff for test #12 would look like:

diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index fd92533ac..87e5001bb 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -17,7 +17,7 @@ helper_test cache

 test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
test_when_finished "
-   git credential-cache exit &&
+   (git credential-cache exit || :) &&
rmdir -p .cache/git/credential/
" &&
test_path_is_missing "$HOME/.git-credential-cache" &&

... and so on for all remaining tests. While this does indeed make all
tests pass, it is not really a solution.

As an aside, while looking to debug this issue, I added the '--debug'
option to the invocation of the 'git-credential-cache--daemon' child
process (see the spawn_daemon() function). This not only fixed the tests,
but also stopped git-credential-cache exiting with a failure. Since the
only effect of passing '--debug' was to suppress the redirection of stderr
to the bit-bucket (/dev/null), I have no idea why this seems to fix the
protocol interaction between git and git-credential-cache--daemon. (I
did think that maybe it was a timing issue, so I tried sleeping before
reading from the daemon on Linux, but that only slowed down the tests!)

All descriptions of the "Connection reset by peer" error, that I could
find, say that the peer had destroyed the connection before the client
attempted to perform I/O on the connection. Since the daemon does not
respond to an "exit" message from the client, it just closes the socket
and deletes the socket file (via the atexit handler), it seems that the
expected result is for the client to receive an EOF.  Indeed, this is
exactly what seems to be happening on Linux. Also a comment in
credential-cache--daemon.c reads:

else if (!strcmp(action.buf, "exit")) {
/*
 * It's important that we clean up our socket first, and then
 * signal the client only once we have finished the cleanup.
 * Calling exit() directly does this, because we clean up in
 * our atexit() handler, and then signal the client when our
 * process actually ends, which closes the socket and gives
 * them EOF.
 */
exit(0);
}

On cygwin this is not the case, at least when not passing --debug to the
daemon, and the read following the "exit" gets an error with errno set
to ECONNRESET.

In order to suppress the fatal exit in this case, check the read error
for an ECONNRESET and return as if no data was read from the daemon.
This effectively converts an ECONNRESET into an EOF.

Signed-off-by: Ramsay Jones 
---
 credential-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/credential-cache.c b/credential-cache.c
index 91550bfb0..13a0b 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -25,7 +25,7 @@ static int send_request(const char *socket, const struct 
strbuf *out)
int r;
 
r = read_in_full(fd, in, sizeof(in));
-   if (r == 0)
+   if (r == 0 || (r < 0 && errno == ECONNRESET))
break;
if (r < 0)
die_errno("read error from cache daemon");
-- 
2.13.0

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

[Junio C Hamano]

Heh, then I'll forward my response and we are even ;-)


-- Forwarded message --
From: Junio C Hamano 
Date: Mon, Jul 10, 2017 at 10:48 AM
Subject: Re: Should "head" also work for "HEAD" on case-insensitive FS?
To: Michael Haggerty 


Michael Haggerty  writes:

> I think the most natural thing would be to use different encoding
> rules for pseudo-refs (references like "HEAD" and "FETCH_HEAD") and
> for other references (those starting with "refs/").
>
> Pseudo-refs (with the partial exception of "HEAD") are quite peculiar
> beasts

I agree with the reasoning, but what I am worried about is that
their handling in the existing refs.c code may be leaky and/or
inconsistent.

What I saw was that a test have ended up with .git/%46%4F%4F when it
was told to create a ref "FOO" (which indicates that "FOO" was
passed to the files backend), which later failed to read it back
because the pseudo_ref handling refs.c wanted to see ".git/FOO" on
the reading side.

Perhaps it is only a bug in t/t1405-main-ref-store.sh?

> But...since we are talking about introducing a new loose reference
> filename encoding, ...

Yes, but that is an encoding detail I do not have to get involved
and folks with platform needs can add more on top---we need to make
sure that the places that encode and decode are identified in the
code first, and the things like "FOO is encoded upon writing because
files-backend is asked to write it, but not decoded because refs.c
thinks it is pseudo-ref and does not give a say to files-backend"
shouldn't be happening before we can start working on the details of
the encoding.  Making a conscious decision that pseudo-refs are left
as-is is OK, but we need to see both reading and writing side
following the same codepath to make that decision, which does not
seem to be the case in the current code.

Re: [RFC PATCH 0/4] Some patches for fsck for missing objects

[Stefan Beller]

On Wed, Jul 26, 2017 at 4:42 PM, brian m. carlson
 wrote:
> On Wed, Jul 26, 2017 at 04:29:58PM -0700, Jonathan Tan wrote:
>> This is an updated version of my previous patch [1], but without the
>> list of promises. It is somewhat different now because fsck cannot just
>> mark all promised objects as HAS_OBJ.
>>
>> This could be adapted and incorporated into patch sets that have objects
>> missing from the local repo.
>>
>> I split this up into 4 patches so that you can see how the changes in
>> fsck are being tested, but eventually these should probably be combined
>> into 1 patch.
>
> I looked at this and I like the direction it's going.  It's pretty
> simple and straightforward, which I also like.

I agree.

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

[Michael Haggerty]

Dang, I just noticed that I hit "reply" rather than "reply-to-all" on
the below email (stupid GMail default). Junio, your response to this
email accordingly went only to me.

Michael

-- Forwarded message --
From: Michael Haggerty 
Date: Mon, Jul 10, 2017 at 7:52 AM
Subject: Re: Should "head" also work for "HEAD" on case-insensitive FS?
To: Junio C Hamano 


On Fri, Jul 7, 2017 at 12:34 AM, Junio C Hamano  wrote:
> [...]
> The exact detail of the encoding used here is immaterial, but I just
> used "encode uppercase letters and % as % followed by two hex",
> which was simple enough.  Usual refs/heads/master and friends will
> not have to be touched when encoded this way.  Perhaps the decoding
> side should be tweaked so that uppercase letters it sees needs to be
> downcased to avoid "refs/heads/Foo" getting returned as "Foo" branch,
> as a "Foo" branch should have been encoded as "refs/heads/%46oo".
>
> Having said that, this patch alone does not quite work yet.
>
>  * In the repository discovery code, we have some logic that
>hard-codes the path in the directory (which is a candidate for
>being a repository) to check, like "refs/" and "HEAD".  In the
>attached illustration patch, files_path_encode() special cases
>"HEAD" so that it is not munged, which is a bit of ugly
>workaround for this.
>
>  * I haven't figured out why, but what refs.c calls "pseudo refs"
>bypasses the files backend layer for some but not all operations,
>which causes t1405-main-ref-store to fail.  The test creates a
>"pseudo ref" FOO and then tries to remove it.  Creation seems to
>follow the files-backend.c and thusly goes through the escaping;
>refs.::refs_delete_ref() however does not consult files-backend.c
>and fails to find and delete .git/FOO, because the creation side
>encoded it as ".git/%46%4F%4F".

I think the most natural thing would be to use different encoding
rules for pseudo-refs (references like "HEAD" and "FETCH_HEAD") and
for other references (those starting with "refs/").

Pseudo-refs (with the partial exception of "HEAD") are quite peculiar
beasts. They sometimes include other information besides the reference
value and IIRC the refs code doesn't have any idea how to write or
read those extra contents. I believe that "HEAD" is the only pseudo
ref for which reflogs are ever written. Pseudo-refs have to match
/[A-Z_]+/ (see 
https://github.com/git/git/blob/8b2efe2a0fd93b8721879f796d848a9ce785647f/refs.c#L169-L173),
so ISTM that there is no need to encode such references' filenames at
all. (It's possible that the pattern could be made even stricter, like
/[A-Z_]+HEAD/.) Moreover, IIRC, such references are never scanned for
(as in for-each-refs) but rather are always asked for by name. So
their names might never have to be *de*coded, either. On the other
hand, when trying to look them up, it would be a good idea to verify
that the requested name satisfies the above naming rule. Other than
that, I believe it would be preferable to leave pseudo-refs untouched
by your new encoding/decoding code.

Whereas other references are typically lower-case, so it makes sense
for lower-case letters to be the ones that are passed through
transparently in such references' filenames (as in your scheme).

But...since we are talking about introducing a new loose reference
filename encoding, I think it would be a good idea to address a couple
of related issues at the same time:

* Some filesystems natively use Unicode, and insist on a particular
Unicode normalization (NFC vs NFD), which might differ from the
"upstream" normalization. So such reference names get munged when
written as loose references. I'm not enough of an expert in Unicode to
know what the best solution is, except for the strong feeling that it
would require some cooperation from the rest of Git to ensure a good
user experience.

* Another bad effect of our current loose reference encoding is that
it prohibits references that D/F conflict with each other (like
"refs/heads/foo" and "refs/heads/foo/bar") because
"$GIT_DIR/refs/heads/foo" can't be a file and a directory at the same
time. Even if we don't want to support that, this problem also
prevents us from storing reflogs for deleted references, which is a
serious flaw. We could solve this problem by encoding "directory"
components of reference names differently than "leaf" components; for
example, the above references could be encoded as
"refs/heads.d/foo.ref" and "refs/heads.d/foo.d/bar.ref".

It'd be nice to solve all of these related problems at the same time,
because whatever encoding we choose now will have to be supported
forever.

Michael

Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink

[Jonathan Nieder]

Hi,

Michael Haggerty wrote:

> One of the tricks that `contrib/workdir/git-new-workdir` plays is to
> making `packed-refs` in the new workdir a symlink to the `packed-refs`
> file in the original repository. Before
> 42dfa7ecef ("commit_packed_refs(): use a staging file separate from
> the lockfile", 2017-06-23), a lockfile was used as the staging file,
> and because the `LOCK_NO_DEREF` was not used, the pointed-to file was
> locked and modified.
>
> But after that commit, the staging file was created using a tempfile,
> with the end result that rewriting the `packed-refs` file in the
> workdir overwrote the symlink rather than the original `packed-refs`
> file.
>
> Change `commit_packed_refs()` to use `get_locked_file_path()` to find
> the path of the file that it should overwrite. Since that path was
> properly resolved when the lockfile was created, this restores the
> pre-42dfa7ecef behavior.
>
> Also add a test case to document this use case and prevent a
> regression like this from recurring.
>
> Signed-off-by: Michael Haggerty 

Reported-by: Dave Walker 

[...]
>  refs/packed-backend.c | 24 ++--
>  t/t3210-pack-refs.sh  | 15 +++
>  2 files changed, 33 insertions(+), 6 deletions(-)

The patch looks good, except for one nit marked below (*).  I'll apply
it locally and ask people to test it, probably tomorrow morning.

Reviewed-by: Jonathan Nieder 

Thanks for writing it.  Patch left unsnipped for reference.

> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index a28befbfa3..59e7d1a509 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -610,19 +610,27 @@ int commit_packed_refs(struct ref_store *ref_store, 
> struct strbuf *err)
>   struct packed_ref_cache *packed_ref_cache =
>   get_packed_ref_cache(refs);
>   int ok;
> + int ret = -1;
>   struct strbuf sb = STRBUF_INIT;
>   FILE *out;
>   struct ref_iterator *iter;
> + char *packed_refs_path;
>  
>   if (!is_lock_file_locked(>lock))
>   die("BUG: commit_packed_refs() called when unlocked");
>  
> - strbuf_addf(, "%s.new", refs->path);
> + /*
> +  * If packed-refs is a symlink, we want to overwrite the
> +  * symlinked-to file, not the symlink itself. Also, put the
> +  * staging file next to it:
> +  */
> + packed_refs_path = get_locked_file_path(>lock);
> + strbuf_addf(, "%s.new", packed_refs_path);
>   if (create_tempfile(>tempfile, sb.buf) < 0) {
>   strbuf_addf(err, "unable to create file %s: %s",
>   sb.buf, strerror(errno));
>   strbuf_release();
> - return -1;
> + goto out;
>   }
>   strbuf_release();
>  
> @@ -660,17 +668,21 @@ int commit_packed_refs(struct ref_store *ref_store, 
> struct strbuf *err)
>   goto error;
>   }
>  
> - if (rename_tempfile(>tempfile, refs->path)) {
> + if (rename_tempfile(>tempfile, packed_refs_path)) {
>   strbuf_addf(err, "error replacing %s: %s",
>   refs->path, strerror(errno));
> - return -1;
> + goto out;
>   }
>  
> - return 0;
> + ret = 0;
> + goto out;
>  
>  error:
>   delete_tempfile(>tempfile);
> - return -1;
> +
> +out:
> + free(packed_refs_path);
> + return ret;
>  }
>  
>  /*
> diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
> index 2bb4b25ed9..0d8a03e2a9 100755
> --- a/t/t3210-pack-refs.sh
> +++ b/t/t3210-pack-refs.sh
> @@ -238,4 +238,19 @@ test_expect_success 'retry acquiring packed-refs.lock' '
>   git -c core.packedrefstimeout=3000 pack-refs --all --prune
>  '
>  
> +test_expect_success 'pack symlinked packed-refs' '

(*) Does this need a SYMLINKS prereq to avoid trouble on Windows?

> + # First make sure that symlinking works when reading:
> + git update-ref refs/heads/loosy refs/heads/master &&
> + git for-each-ref >all-refs-before &&
> + mv .git/packed-refs .git/my-deviant-packed-refs &&
> + ln -s my-deviant-packed-refs .git/packed-refs &&
> + git for-each-ref >all-refs-linked &&
> + test_cmp all-refs-before all-refs-linked &&
> + git pack-refs --all --prune &&
> + git for-each-ref >all-refs-packed &&
> + test_cmp all-refs-before all-refs-packed &&
> + test -h .git/packed-refs &&
> + test "$(readlink .git/packed-refs)" = "my-deviant-packed-refs"
> +'
> +
>  test_done

Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink

[Stefan Beller]

On Wed, Jul 26, 2017 at 4:39 PM, Michael Haggerty  wrote:
> One of the tricks that `contrib/workdir/git-new-workdir` plays is to
> making `packed-refs` in the new workdir a symlink to the `packed-refs`
> file in the original repository. Before
> 42dfa7ecef ("commit_packed_refs(): use a staging file separate from
> the lockfile", 2017-06-23), a lockfile was used as the staging file,
> and because the `LOCK_NO_DEREF` was not used, the pointed-to file was
> locked and modified.
>
> But after that commit, the staging file was created using a tempfile,
> with the end result that rewriting the `packed-refs` file in the
> workdir overwrote the symlink rather than the original `packed-refs`
> file.
>
> Change `commit_packed_refs()` to use `get_locked_file_path()` to find
> the path of the file that it should overwrite. Since that path was
> properly resolved when the lockfile was created, this restores the
> pre-42dfa7ecef behavior.
>
> Also add a test case to document this use case and prevent a
> regression like this from recurring.
>
> Signed-off-by: Michael Haggerty 
> ---
> Sorry for the slow response; I've been traveling and very busy.
>
> I didn't even know that a symlinked `packed-refs` file is a thing. The
> attached patch should fix the problem. It applies on top of v3 of
> mh/packed-ref-store [1] (which is already in next).

Thanks for providing a fix.
Code looks good to me, I wonder if the test needs SYMLINKS
special treatment though?

Stefan

Re: [PATCHv2] builtin/blame: highlight interesting things

[Stefan Beller]

On Wed, Jul 26, 2017 at 4:29 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> When using git-blame lots of lines contain redundant information, for
>> example in hunks that consist of multiple lines, the metadata (commit name,
>> author) are repeated. A reader may not be interested in those, so darken
>> (commit, author) information that is the same as in the previous line.
>>
>> Choose a different approach for dates and imitate a 'temperature cool down'
>> for the dates. Compute the time range of all involved blamed commits
>> and then color
>>  * lines of old commits dark (aged 0-50% in that time range)
>>  * lines of medium age normal (50-80%)
>>  * lines of new age red (80-95%)
>>  * lines just introduced bright yellow (95-100%)
>>
>> Signed-off-by: Stefan Beller 
>> ---
>>
>>   I played around with it a bit more, using a different color scheme
>>   for dates, http://i.imgur.com/redhaLi.png
>
> I do agree with what this one tries to do, in that a block of lines
> tend to share the same metainfo as they come from the same commit
> and it is distracting to see them repeatedly---doing something to
> make their "these are in one group" nature stand out will give us a
> much better presentation.

Well, we could also try a "zebra"  as in sb/diff-color-move to show blocks
with the same fancy border detection.

> But does this particular implementation work well for people who use
> black in on white background?  "Darken to make it less distracting"
> may not work on both white-on-black and black-on-white users.

correct. Once we have a shared understanding what the
"interesting things" are and how to handle them, I would add
color.blame. options to make it configurable.

> "Show the background only by replacing the letters with SP for
> metainfo that are same as previous line" would work for folks from
> either camp, I would imagine.  And that should be a single feature,
> that can be enabled independently from the age based coloring.

True, I had that as the very first step of this experiment, I lost the
patch for it, but could redo it for presentation and discussion.

My impression was that this would remove _too_ much, e.g. if
a commit spans more than one screen, you may not see the first
line, but only blank space.

> The age coloring is much harder to make it work for folks from both
> camps at the same time with the same color selection.  Yellow on
> white would be terribly unreadable for black-on-white folks, for
> example.

Configuration is key here, I would think, both in the color space, as well
as in the selection space. One could imagine that other people would
rather have a defined time span, e.g. hard coding "2 weeks/one quarter/
more than a year" or relate that time span to the project history instead
of the file history.

> If you make "make it less distracting by blanking them out (not
> 'darken them')" feature without the age coloring, that can be usable
> immediately by folks from both camps, even if you cannot find a way
> to do the age coloring that would satisfy both groups.  One group
> can just leave the knob off and not use the age coloring, while the
> other group can use it and people from both camps will be happier
> than the status quo.

ok.

Thanks,
Stefan

Re: [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

[Michael Haggerty]

I just submitted a patch [1] that fixes the `packed-refs`-is-a-symlink
problem in combination with git-new-workdir.

I haven't considered any possible problems related to
`core.preferSymlinkRefs`. That behavior should be unaffected by the
packed-ref-store patch series and I'm not very interested in working
on it.

Michael

[1] 
https://public-inbox.org/git/d0da02a8b6f0272fa70ae3b1dc80fee6c6ee8d18.150803.git.mhag...@alum.mit.edu/

On Mon, Jul 24, 2017 at 2:43 PM, Junio C Hamano  wrote:
> Jeff King  writes:
>
>> On Mon, Jul 24, 2017 at 10:09:15AM -0700, Jonathan Nieder wrote:
>>
>>> Jeff King wrote:
>>>
>>> > This seems like the correct path to me. If the existing behavior is to
>>> > lock the referring symref, that seems like a violation of the lock
>>> > procedure in the first place. Because if "A" points to "B", we take
>>> > "A.lock" and then modify "B". But "B" may have any number of "A" links
>>> > pointing to it, eliminating the purpose of the lock.
>>> >
>>> > I thought we already did this, though. And that modifying HEAD (which
>>> > might be a symlink) required LOCK_NO_DEREF.
>>>
>>> Yes, I believe the lockfile API already does so.  Since this patch
>>> creates a ".new" file, not using the lockfile API, it doesn't benefit
>>> from that, though.
>>
>> Ah, I see. This bug makes much more sense, then. And I agree doubly that
>> matching the lock-code's behavior is the right thing to do.
>
> OK.  Anybody wants to take a crack at it (it is of lower priority to
> me during the -rc freeze to deal with problems in topics on 'next'
> or higher)?
>
> Thanks.

Re: [RFC PATCH 0/4] Some patches for fsck for missing objects

[brian m. carlson]

On Wed, Jul 26, 2017 at 04:29:58PM -0700, Jonathan Tan wrote:
> This is an updated version of my previous patch [1], but without the
> list of promises. It is somewhat different now because fsck cannot just
> mark all promised objects as HAS_OBJ.
> 
> This could be adapted and incorporated into patch sets that have objects
> missing from the local repo.
> 
> I split this up into 4 patches so that you can see how the changes in
> fsck are being tested, but eventually these should probably be combined
> into 1 patch.

I looked at this and I like the direction it's going.  It's pretty
simple and straightforward, which I also like.

What I'd recommend is that instead of making lazyObject a string, we
make it an integer representing a protocol version.  We then add a
different config setting that is the actual program to invoke, using the
given protocol version.  This lets us change the way we speak to the
tool without breaking backwards compatibility, and it also allows us to
simply check the lazyObject script for supported protocols up front.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

[PATCH] packed_ref_store: handle a packed-refs file that is a symlink

[Michael Haggerty]

One of the tricks that `contrib/workdir/git-new-workdir` plays is to
making `packed-refs` in the new workdir a symlink to the `packed-refs`
file in the original repository. Before
42dfa7ecef ("commit_packed_refs(): use a staging file separate from
the lockfile", 2017-06-23), a lockfile was used as the staging file,
and because the `LOCK_NO_DEREF` was not used, the pointed-to file was
locked and modified.

But after that commit, the staging file was created using a tempfile,
with the end result that rewriting the `packed-refs` file in the
workdir overwrote the symlink rather than the original `packed-refs`
file.

Change `commit_packed_refs()` to use `get_locked_file_path()` to find
the path of the file that it should overwrite. Since that path was
properly resolved when the lockfile was created, this restores the
pre-42dfa7ecef behavior.

Also add a test case to document this use case and prevent a
regression like this from recurring.

Signed-off-by: Michael Haggerty 
---
Sorry for the slow response; I've been traveling and very busy.

I didn't even know that a symlinked `packed-refs` file is a thing. The
attached patch should fix the problem. It applies on top of v3 of
mh/packed-ref-store [1] (which is already in next).

Michael

[1] http://public-inbox.org/git/cover.1498933362.git.mhag...@alum.mit.edu/

 refs/packed-backend.c | 24 ++--
 t/t3210-pack-refs.sh  | 15 +++
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index a28befbfa3..59e7d1a509 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -610,19 +610,27 @@ int commit_packed_refs(struct ref_store *ref_store, 
struct strbuf *err)
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(refs);
int ok;
+   int ret = -1;
struct strbuf sb = STRBUF_INIT;
FILE *out;
struct ref_iterator *iter;
+   char *packed_refs_path;
 
if (!is_lock_file_locked(>lock))
die("BUG: commit_packed_refs() called when unlocked");
 
-   strbuf_addf(, "%s.new", refs->path);
+   /*
+* If packed-refs is a symlink, we want to overwrite the
+* symlinked-to file, not the symlink itself. Also, put the
+* staging file next to it:
+*/
+   packed_refs_path = get_locked_file_path(>lock);
+   strbuf_addf(, "%s.new", packed_refs_path);
if (create_tempfile(>tempfile, sb.buf) < 0) {
strbuf_addf(err, "unable to create file %s: %s",
sb.buf, strerror(errno));
strbuf_release();
-   return -1;
+   goto out;
}
strbuf_release();
 
@@ -660,17 +668,21 @@ int commit_packed_refs(struct ref_store *ref_store, 
struct strbuf *err)
goto error;
}
 
-   if (rename_tempfile(>tempfile, refs->path)) {
+   if (rename_tempfile(>tempfile, packed_refs_path)) {
strbuf_addf(err, "error replacing %s: %s",
refs->path, strerror(errno));
-   return -1;
+   goto out;
}
 
-   return 0;
+   ret = 0;
+   goto out;
 
 error:
delete_tempfile(>tempfile);
-   return -1;
+
+out:
+   free(packed_refs_path);
+   return ret;
 }
 
 /*
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 2bb4b25ed9..0d8a03e2a9 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -238,4 +238,19 @@ test_expect_success 'retry acquiring packed-refs.lock' '
git -c core.packedrefstimeout=3000 pack-refs --all --prune
 '
 
+test_expect_success 'pack symlinked packed-refs' '
+   # First make sure that symlinking works when reading:
+   git update-ref refs/heads/loosy refs/heads/master &&
+   git for-each-ref >all-refs-before &&
+   mv .git/packed-refs .git/my-deviant-packed-refs &&
+   ln -s my-deviant-packed-refs .git/packed-refs &&
+   git for-each-ref >all-refs-linked &&
+   test_cmp all-refs-before all-refs-linked &&
+   git pack-refs --all --prune &&
+   git for-each-ref >all-refs-packed &&
+   test_cmp all-refs-before all-refs-packed &&
+   test -h .git/packed-refs &&
+   test "$(readlink .git/packed-refs)" = "my-deviant-packed-refs"
+'
+
 test_done
-- 
2.11.0

[RFC PATCH 0/4] Some patches for fsck for missing objects

[Jonathan Tan]

This is an updated version of my previous patch [1], but without the
list of promises. It is somewhat different now because fsck cannot just
mark all promised objects as HAS_OBJ.

This could be adapted and incorporated into patch sets that have objects
missing from the local repo.

I split this up into 4 patches so that you can see how the changes in
fsck are being tested, but eventually these should probably be combined
into 1 patch.

[1] 
https://public-inbox.org/git/3420d9ae9ef86b78af1abe721891233e3f5865a2.1500508695.git.jonathanta...@google.com/

Jonathan Tan (4):
  environment, fsck: introduce lazyobject extension
  fsck: support refs pointing to lazy objects
  fsck: support referenced lazy objects
  fsck: support lazy objects as CLI argument

 Documentation/technical/repository-version.txt |  7 ++
 builtin/fsck.c | 23 ++-
 cache.h|  2 +
 environment.c  |  1 +
 setup.c|  7 +-
 t/t0410-lazy-object.sh | 95 ++
 6 files changed, 133 insertions(+), 2 deletions(-)
 create mode 100755 t/t0410-lazy-object.sh

-- 
2.14.0.rc0.400.g1c36432dff-goog

[RFC PATCH 3/4] fsck: support referenced lazy objects

[Jonathan Tan]

Teach fsck to not treat missing objects indirectly pointed to by refs as
an error when extensions.lazyobject is set.

Signed-off-by: Jonathan Tan 
---
 builtin/fsck.c | 11 +++
 t/t0410-lazy-object.sh | 27 +++
 2 files changed, 38 insertions(+)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index e29ff760b..238532cc2 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -149,6 +149,15 @@ static int mark_object(struct object *obj, int type, void 
*data, struct fsck_opt
return 0;
obj->flags |= REACHABLE;
if (!(obj->flags & HAS_OBJ)) {
+   if (repository_format_lazy_object)
+   /*
+* Return immediately; this is not an error, and further
+* recursion does not need to be performed on this
+* object since it is missing (so it does not need to be
+* added to "pending").
+*/
+   return 0;
+
if (parent && !has_object_file(>oid)) {
printf("broken link from %7s %s\n",
 printable_type(parent), 
describe_object(parent));
@@ -212,6 +221,8 @@ static void check_reachable_object(struct object *obj)
 * do a full fsck
 */
if (!(obj->flags & HAS_OBJ)) {
+   if (repository_format_lazy_object)
+   return;
if (has_sha1_pack(obj->oid.hash))
return; /* it is in pack - forget about it */
printf("missing %s %s\n", printable_type(obj),
diff --git a/t/t0410-lazy-object.sh b/t/t0410-lazy-object.sh
index 00e1b4a88..45f665a15 100755
--- a/t/t0410-lazy-object.sh
+++ b/t/t0410-lazy-object.sh
@@ -49,4 +49,31 @@ test_expect_success '...but succeeds if lazyobject is set' '
git -C repo fsck
 '
 
+test_expect_success 'fsck fails on lazy object indirectly pointed to by ref' '
+   rm -rf repo &&
+   test_create_repo repo &&
+   test_commit -C repo 1 &&
+   test_commit -C repo 2 &&
+   test_commit -C repo 3 &&
+   git -C repo tag -a annotated_tag -m "annotated tag" &&
+
+   C=$(git -C repo rev-parse 1) &&
+   T=$(git -C repo rev-parse 2^{tree}) &&
+   B=$(git hash-object repo/3.t) &&
+   AT=$(git -C repo rev-parse annotated_tag) &&
+
+   # missing commit, tree, blob, and tag
+   delete_object repo "$C" &&
+   delete_object repo "$T" &&
+   delete_object repo "$B" &&
+   delete_object repo "$AT" &&
+   test_must_fail git -C repo fsck
+'
+
+test_expect_success '...but succeeds if lazyobject is set' '
+   git -C repo config core.repositoryformatversion 1 &&
+   git -C repo config extensions.lazyobject "arbitrary string" &&
+   git -C repo fsck
+'
+
 test_done
-- 
2.14.0.rc0.400.g1c36432dff-goog

[RFC PATCH 2/4] fsck: support refs pointing to lazy objects

[Jonathan Tan]

Teach fsck to not treat refs with missing targets as an error when
extensions.lazyobject is set.

For the purposes of warning about no default refs, such refs are still
treated as legitimate refs.

Signed-off-by: Jonathan Tan 
---
 builtin/fsck.c |  8 
 t/t0410-lazy-object.sh | 20 
 2 files changed, 28 insertions(+)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 1cfb8d98c..e29ff760b 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -438,6 +438,14 @@ static int fsck_handle_ref(const char *refname, const 
struct object_id *oid,
 
obj = parse_object(oid);
if (!obj) {
+   if (repository_format_lazy_object) {
+   /*
+* Increment default_refs anyway, because this is a
+* valid ref.
+*/
+   default_refs++;
+   return 0;
+   }
error("%s: invalid sha1 pointer %s", refname, oid_to_hex(oid));
errors_found |= ERROR_REACHABLE;
/* We'll continue with the rest despite the error.. */
diff --git a/t/t0410-lazy-object.sh b/t/t0410-lazy-object.sh
index 36442531f..00e1b4a88 100755
--- a/t/t0410-lazy-object.sh
+++ b/t/t0410-lazy-object.sh
@@ -29,4 +29,24 @@ test_expect_success '...but succeeds if lazyobject is set' '
git -C repo fsck
 '
 
+test_expect_success 'fsck fails on lazy object pointed to by ref' '
+   rm -rf repo &&
+   test_create_repo repo &&
+   test_commit -C repo 1 &&
+
+   A=$(git -C repo commit-tree -m a HEAD^{tree}) &&
+
+   # Reference $A only from ref, and delete it
+   git -C repo branch mybranch "$A" &&
+   delete_object repo "$A" &&
+
+   test_must_fail git -C repo fsck
+'
+
+test_expect_success '...but succeeds if lazyobject is set' '
+   git -C repo config core.repositoryformatversion 1 &&
+   git -C repo config extensions.lazyobject "arbitrary string" &&
+   git -C repo fsck
+'
+
 test_done
-- 
2.14.0.rc0.400.g1c36432dff-goog

[RFC PATCH 1/4] environment, fsck: introduce lazyobject extension

[Jonathan Tan]

Currently, Git does not support repos with very large numbers of objects
or repos that wish to minimize manipulation of certain blobs (for
example, because they are very large) very well, even if the user
operates mostly on part of the repo, because Git is designed on the
assumption that every referenced object is available somewhere in the
repo storage.

Introduce a new repository extension option "extensions.lazyobject", of
data type string. If this is set in a repository, Git will tolerate
objects being missing in that repository.  When Git needs those objects,
it will invoke the command in that option.

Teach fsck about the new state of affairs. In this commit, teach fsck
that missing objects referenced from the reflog are not an error case;
in future commits, fsck will be taught about other cases.

Signed-off-by: Jonathan Tan 
---
 Documentation/technical/repository-version.txt |  7 ++
 builtin/fsck.c |  2 +-
 cache.h|  2 ++
 environment.c  |  1 +
 setup.c|  7 +-
 t/t0410-lazy-object.sh | 32 ++
 6 files changed, 49 insertions(+), 2 deletions(-)
 create mode 100755 t/t0410-lazy-object.sh

diff --git a/Documentation/technical/repository-version.txt 
b/Documentation/technical/repository-version.txt
index 00ad37986..39e362543 100644
--- a/Documentation/technical/repository-version.txt
+++ b/Documentation/technical/repository-version.txt
@@ -86,3 +86,10 @@ for testing format-1 compatibility.
 When the config key `extensions.preciousObjects` is set to `true`,
 objects in the repository MUST NOT be deleted (e.g., by `git-prune` or
 `git repack -d`).
+
+`lazyObject`
+~
+
+When the config key `extensions.lazyObject` is set to a string, Git
+tolerates objects being missing in the repository. This string contains
+the command to be run whenever a missing object is needed.
diff --git a/builtin/fsck.c b/builtin/fsck.c
index fb0947009..1cfb8d98c 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -402,7 +402,7 @@ static void fsck_handle_reflog_oid(const char *refname, 
struct object_id *oid,
xstrfmt("%s@{%"PRItime"}", refname, 
timestamp));
obj->flags |= USED;
mark_object_reachable(obj);
-   } else {
+   } else if (!repository_format_lazy_object) {
error("%s: invalid reflog entry %s", refname, 
oid_to_hex(oid));
errors_found |= ERROR_REACHABLE;
}
diff --git a/cache.h b/cache.h
index 6c8242340..9e6bc0a21 100644
--- a/cache.h
+++ b/cache.h
@@ -853,10 +853,12 @@ extern int grafts_replace_parents;
 #define GIT_REPO_VERSION 0
 #define GIT_REPO_VERSION_READ 1
 extern int repository_format_precious_objects;
+extern char *repository_format_lazy_object;
 
 struct repository_format {
int version;
int precious_objects;
+   char *lazy_object;
int is_bare;
char *work_tree;
struct string_list unknown_extensions;
diff --git a/environment.c b/environment.c
index 3fd4b1084..cd8ef2897 100644
--- a/environment.c
+++ b/environment.c
@@ -27,6 +27,7 @@ int warn_ambiguous_refs = 1;
 int warn_on_object_refname_ambiguity = 1;
 int ref_paranoia = -1;
 int repository_format_precious_objects;
+char *repository_format_lazy_object;
 const char *git_commit_encoding;
 const char *git_log_output_encoding;
 const char *apply_default_whitespace;
diff --git a/setup.c b/setup.c
index 860507e1f..94cfde3cc 100644
--- a/setup.c
+++ b/setup.c
@@ -425,7 +425,11 @@ static int check_repo_format(const char *var, const char 
*value, void *vdata)
;
else if (!strcmp(ext, "preciousobjects"))
data->precious_objects = git_config_bool(var, value);
-   else
+   else if (!strcmp(ext, "lazyobject")) {
+   if (!value)
+   return config_error_nonbool(var);
+   data->lazy_object = xstrdup(value);
+   } else
string_list_append(>unknown_extensions, ext);
} else if (strcmp(var, "core.bare") == 0) {
data->is_bare = git_config_bool(var, value);
@@ -468,6 +472,7 @@ static int check_repository_format_gently(const char 
*gitdir, int *nongit_ok)
}
 
repository_format_precious_objects = candidate.precious_objects;
+   repository_format_lazy_object = candidate.lazy_object;
string_list_clear(_extensions, 0);
if (!has_common) {
if (candidate.is_bare != -1) {
diff --git a/t/t0410-lazy-object.sh b/t/t0410-lazy-object.sh
new file mode 100755
index 0..36442531f
--- /dev/null
+++ b/t/t0410-lazy-object.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+
+test_description='lazy object'

[RFC PATCH 4/4] fsck: support lazy objects as CLI argument

[Jonathan Tan]

Teach fsck to not treat missing objects provided on the CLI as an error
when extensions.lazyobject is set.

Signed-off-by: Jonathan Tan 
---
 builtin/fsck.c |  2 ++
 t/t0410-lazy-object.sh | 16 
 2 files changed, 18 insertions(+)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 238532cc2..592e64172 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -755,6 +755,8 @@ int cmd_fsck(int argc, const char **argv, const char 
*prefix)
struct object *obj = lookup_object(oid.hash);
 
if (!obj || !(obj->flags & HAS_OBJ)) {
+   if (repository_format_lazy_object)
+   continue;
error("%s: object missing", oid_to_hex());
errors_found |= ERROR_OBJECT;
continue;
diff --git a/t/t0410-lazy-object.sh b/t/t0410-lazy-object.sh
index 45f665a15..3ac61c1c5 100755
--- a/t/t0410-lazy-object.sh
+++ b/t/t0410-lazy-object.sh
@@ -76,4 +76,20 @@ test_expect_success '...but succeeds if lazyobject is set' '
git -C repo fsck
 '
 
+test_expect_success 'fsck fails on lazy object directly given in command-line' 
'
+   rm -rf repo &&
+   test_create_repo repo &&
+   test_commit -C repo 1 &&
+   HASH=$(git hash-object repo/1.t) &&
+   delete_object repo "$HASH" &&
+
+   test_must_fail git -C repo fsck "$HASH"
+'
+
+test_expect_success '...but succeeds if lazyobject is set' '
+   git -C repo config core.repositoryformatversion 1 &&
+   git -C repo config extensions.lazyobject "arbitrary string" &&
+   git -C repo fsck "$HASH"
+'
+
 test_done
-- 
2.14.0.rc0.400.g1c36432dff-goog

Re: [ANNOUNCE] Git v2.14.0-rc1

[brian m. carlson]

On Mon, Jul 24, 2017 at 03:44:23PM -0700, Junio C Hamano wrote:
> A release candidate Git v2.14.0-rc1 is now available for testing
> at the usual places.  It is comprised of 708 non-merge commits
> since v2.13.0, contributed by 61 people, 14 of which are new faces.
> 
> The tarballs are found at:
> 
> https://www.kernel.org/pub/software/scm/git/testing/

I noticed these tarballs are signed with SHA-1.  Since we're working on
adding additional hash support for SHA-256, I was wondering if you might
consider using SHA-256 instead.

This works all the way back to the GnuPG shipped with CentOS 4, so there
should be no compatibility issues.  If you're using a reasonably
up-to-date version of GnuPG 1.4 or 2, you should be able to simply drop
the following line in ~/.gnupg/gpg.conf and have things work:

personal-digest-preferences SHA256 SHA384 SHA512

You can also change the order to suit your liking; SHA-384 and SHA-512
support still goes back to at least 2007 (CentOS 5).
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCHv2] builtin/blame: highlight interesting things

[Junio C Hamano]

Stefan Beller  writes:

> When using git-blame lots of lines contain redundant information, for
> example in hunks that consist of multiple lines, the metadata (commit name,
> author) are repeated. A reader may not be interested in those, so darken
> (commit, author) information that is the same as in the previous line.
>
> Choose a different approach for dates and imitate a 'temperature cool down'
> for the dates. Compute the time range of all involved blamed commits
> and then color
>  * lines of old commits dark (aged 0-50% in that time range)
>  * lines of medium age normal (50-80%)
>  * lines of new age red (80-95%)
>  * lines just introduced bright yellow (95-100%)
>
> Signed-off-by: Stefan Beller 
> ---
>
>   I played around with it a bit more, using a different color scheme
>   for dates, http://i.imgur.com/redhaLi.png

I do agree with what this one tries to do, in that a block of lines
tend to share the same metainfo as they come from the same commit
and it is distracting to see them repeatedly---doing something to
make their "these are in one group" nature stand out will give us a
much better presentation.

But does this particular implementation work well for people who use
black in on white background?  "Darken to make it less distracting"
may not work on both white-on-black and black-on-white users.

"Show the background only by replacing the letters with SP for
metainfo that are same as previous line" would work for folks from
either camp, I would imagine.  And that should be a single feature,
that can be enabled independently from the age based coloring.

The age coloring is much harder to make it work for folks from both
camps at the same time with the same color selection.  Yellow on
white would be terribly unreadable for black-on-white folks, for
example.

If you make "make it less distracting by blanking them out (not
'darken them')" feature without the age coloring, that can be usable
immediately by folks from both camps, even if you cannot find a way
to do the age coloring that would satisfy both groups.  One group
can just leave the knob off and not use the age coloring, while the
other group can use it and people from both camps will be happier
than the status quo.

Thanks.

[PATCHv2] builtin/blame: highlight interesting things

[Stefan Beller]

When using git-blame lots of lines contain redundant information, for
example in hunks that consist of multiple lines, the metadata (commit name,
author) are repeated. A reader may not be interested in those, so darken
(commit, author) information that is the same as in the previous line.

Choose a different approach for dates and imitate a 'temperature cool down'
for the dates. Compute the time range of all involved blamed commits
and then color
 * lines of old commits dark (aged 0-50% in that time range)
 * lines of medium age normal (50-80%)
 * lines of new age red (80-95%)
 * lines just introduced bright yellow (95-100%)

Signed-off-by: Stefan Beller 
---

  I played around with it a bit more, using a different color scheme
  for dates, http://i.imgur.com/redhaLi.png


 builtin/blame.c | 140 ++--
 color.h |   1 +
 2 files changed, 118 insertions(+), 23 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index bda1a78726..552ea8e6f7 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -7,6 +7,7 @@
 
 #include "cache.h"
 #include "config.h"
+#include "color.h"
 #include "builtin.h"
 #include "commit.h"
 #include "diff.h"
@@ -283,11 +284,14 @@ static void found_guilty_entry(struct blame_entry *ent, 
void *data)
 }
 
 static const char *format_time(timestamp_t time, const char *tz_str,
-  int show_raw_time)
+  int show_raw_time, const char *color,
+  const char *reset)
 {
static struct strbuf time_buf = STRBUF_INIT;
 
strbuf_reset(_buf);
+   if (color)
+   strbuf_addstr(_buf, color);
if (show_raw_time) {
strbuf_addf(_buf, "%"PRItime" %s", time, tz_str);
}
@@ -307,6 +311,8 @@ static const char *format_time(timestamp_t time, const char 
*tz_str,
 time_width++)
strbuf_addch(_buf, ' ');
}
+   if (reset)
+   strbuf_addstr(_buf, reset);
return time_buf.buf;
 }
 
@@ -319,7 +325,8 @@ static const char *format_time(timestamp_t time, const char 
*tz_str,
 #define OUTPUT_SHOW_SCORE  0100
 #define OUTPUT_NO_AUTHOR   0200
 #define OUTPUT_SHOW_EMAIL  0400
-#define OUTPUT_LINE_PORCELAIN 01000
+#define OUTPUT_LINE_PORCELAIN  01000
+#define OUTPUT_SHOW_HIGHLIGHT  02000
 
 static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
 {
@@ -367,19 +374,62 @@ static void emit_porcelain(struct blame_scoreboard *sb, 
struct blame_entry *ent,
putchar('\n');
 }
 
-static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, 
int opt)
+static void emit_other(struct blame_scoreboard *sb,
+  struct blame_entry *ent,
+  struct blame_entry *prev,
+  int opt,
+  timestamp_t min_t,
+  timestamp_t max_t)
 {
int cnt;
const char *cp;
struct blame_origin *suspect = ent->suspect;
-   struct commit_info ci;
+   struct commit_info ci, prev_ci;
char hex[GIT_MAX_HEXSZ + 1];
int show_raw_time = !!(opt & OUTPUT_RAW_TIMESTAMP);
+   int prev_same_field = 0;
+   const char *use_color, *reset_color = GIT_COLOR_RESET;
+   const char *date_color = NULL;
 
get_commit_info(suspect->commit, , 1);
oid_to_hex_r(hex, >commit->object.oid);
 
cp = blame_nth_line(sb, ent->lno);
+
+   commit_info_init(_ci);
+   if ((opt & OUTPUT_SHOW_HIGHLIGHT) && prev) {
+   get_commit_info(prev->suspect->commit, _ci, 1);
+   if ((opt & OUTPUT_SHOW_SCORE) && ent->score == prev->score)
+   prev_same_field |= OUTPUT_SHOW_SCORE;
+   if ((opt & OUTPUT_SHOW_NAME) && prev->suspect && 
!strcmp(suspect->path, prev->suspect->path))
+   prev_same_field |= OUTPUT_SHOW_NAME;
+   if ((opt & OUTPUT_SHOW_NUMBER) &&
+   ent->s_lno == prev->s_lno + prev->num_lines - 1)
+   prev_same_field |= OUTPUT_SHOW_NUMBER;
+   if (!(opt & OUTPUT_NO_AUTHOR)) {
+   if (((opt & OUTPUT_SHOW_EMAIL) &&
+!strcmp(ci.author_mail.buf, 
prev_ci.author_mail.buf)) ||
+   !strcmp(ci.author.buf, prev_ci.author.buf))
+   prev_same_field |= OUTPUT_NO_AUTHOR;
+   }
+   }
+   if (opt & OUTPUT_SHOW_HIGHLIGHT) {
+   if (max_t == min_t) {
+   date_color = GIT_COLOR_NORMAL;
+   } else {
+   float score = 1.0 * (ci.author_time - min_t);
+   score /= (1.0 * (max_t - min_t));
+   if (score > 0.95)
+   date_color = GIT_COLOR_BOLD_YELLOW;
+   else if (score > 0.8)
+  

Re: Change in output as a result of patch

[Junio C Hamano]

Kaartic Sivaraam  writes:

> b347d06bf09 (branch: deprecate --set-upstream and show help if we detect 
> possible mistaken use,
>  Thu Aug 30 19:23:13 2012)
>
> Is there any possibility for it to be removed some time in the near
> future?
>
> I'm asking this because IIRC, the 'attr_only' parameter of
> "validate_new_branchname" was introduced to fix a regression
> (fa7993767560) caused by the "--set-upstream" option. In case it has
> been planned to be removed some time soon, it could make the word
> easier (?), not sure though.

I suspect that it would not make the refactoring that much less
work, but you are right---it is about time we started looking into
removing the --set-upstream optin whose 5th anniversary after
deprecation is only one month away.

Re: [RFC PATCH 0/5] Add option to autostage changes when continuing a rebase

[Junio C Hamano]

Junio C Hamano  writes:

> Hmph, this is interesting.
>
> "git rebase" does take "--rerere-autoupdate" option from the command
> line, and propagates it to a later invocation of "rebase --continue"
> by storing the value to $state_dir/allow_rerere_autoupdate file and
> reading the value from it.  $allow_rerere_autoupdate shell variable
> is used to hold the setting.  
>
> I'd expect that this variable to be used in invocations of "git am"
> in git-rebase--am.sh; but that does not seem to be the case.  I
> wonder if this was once working but over time we broke the feature
> without anybody noticing it, or if the support was added but not
> completed and the feature was a no-op from the beginning?

At least in v1.7.0 when doing "rebase -m", the rerere-autoupdate was
plumbed correctly through to the invocation of "git merge" that is
done inside git-rebase.sh.  I do not see the same option passed down
to the invocation of "git am", so perhaps nobody cared back then
about rerere during "git rebase" that does not use "git am" backend,
even though "git am" itself were capable of talking the option.

In any case, if you corrected the existing "git rebase" and its
backend so that "--rerere-autoupdate" works as advertised, I think
you are already 80% there without adding a yet another option, as I
suspect that the most of the need for avoiding "git add" during a
"git rebase" session is during a conflict resolution, and allowing
"rerere" to automatically update the index with auto-resolution will
leave _only_ changes to the paths the end user actually needs to
take a look and manually fix still not in the index.  And from the
workflow point of view, encouraging them to "git add" their manual
resolution after they are satisified with their changes by not doing
"git add" blindly for all changes, like your --autostage" does, is
probably a good thing.

Re: [RFC PATCH 0/5] Add option to autostage changes when continuing a rebase

[Junio C Hamano]

Junio C Hamano  writes:

> Phillip Wood  writes:
>
>> From: Phillip Wood 
>>
>> These patches add an '--autostage' option (and corresponding config
>> variable) to 'rebase --continue' that will stage any unstaged changes
>> before continuing. This saves the user having to type 'git add' before
>> running 'git rebase --continue'.
>
> I wonder if this interacts with existing rerere.autoupdate
> configuration variable and if so how (i.e. would they conflict and
> fight with each other?  would they work well together?  would one of
> them make the other unnecessary?).  In any case, they look closely
> related and perhaps should be named similarly.
>
> I even have a suspicion that they may be essentially doing the same
> thing.
>
> For a previous discussion, you start from here:
>
>   https://public-inbox.org/git/7vej6xb4lr@gitster.siamese.dyndns.org/#t
>
> and for the context, look at the original post by Ingo, to which the
> above message is a response to.
>
> Thanks.

Hmph, this is interesting.

"git rebase" does take "--rerere-autoupdate" option from the command
line, and propagates it to a later invocation of "rebase --continue"
by storing the value to $state_dir/allow_rerere_autoupdate file and
reading the value from it.  $allow_rerere_autoupdate shell variable
is used to hold the setting.  

I'd expect that this variable to be used in invocations of "git am"
in git-rebase--am.sh; but that does not seem to be the case.  I
wonder if this was once working but over time we broke the feature
without anybody noticing it, or if the support was added but not
completed and the feature was a no-op from the beginning?

Re: [PATCH 09/15] submodule: remove submodule_config callback routine

[Junio C Hamano]

Brandon Williams  writes:

> Remove the last remaining caller of 'submodule_config()' as well as the
> function itself.
>
> With 'submodule_config()' being removed the submodule-config API can be
> a little simpler as callers don't need to worry about whether or not
> they need to overlay the repository's config on top of the
> submodule-config.  This also makes it more difficult to accidentally
> add non-submodule specific configuration to the .gitmodules file.

Nice.

Re: [PATCH 03/15] add, reset: ensure submodules can be added or reset

[Junio C Hamano]

Stefan Beller  writes:

> On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams  wrote:
>> Commit aee9c7d65 (Submodules: Add the new "ignore" config option for
>> diff and status) ...
>
> introduced in 2010, so quite widely spread.
>
>> ...  introduced the ignore configuration option for
>> submodules so that configured submodules could be omitted from the
>> status and diff commands.  Because this flag is respected in the diff
>> machinery it has the unintended consequence of potentially prohibiting
>> users from adding or resetting a submodule, even when a path to the
>> submodule is explicitly given.
>>
>> Ensure that submodules can be added or set, even if they are configured
>> to be ignored, by setting the `DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG` diff
>> flag.
>>
>> Signed-off-by: Brandon Williams 
>> ---
>>  builtin/add.c   | 1 +
>>  builtin/reset.c | 1 +
>>  2 files changed, 2 insertions(+)
>>
>> diff --git a/builtin/add.c b/builtin/add.c
>> index e888fb8c5..6f271512f 100644
>> --- a/builtin/add.c
>> +++ b/builtin/add.c
>> @@ -116,6 +116,7 @@ int add_files_to_cache(const char *prefix,
>> rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
>> rev.diffopt.format_callback = update_callback;
>> rev.diffopt.format_callback_data = 
>> +   rev.diffopt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
>
>
> This flag occurs once in the code base, with the comment:
> /*
>  * Unless the user did explicitly request a submodule
>  * ignore mode by passing a command line option we do
>  * not ignore any changed submodule SHA-1s when
>  * comparing index and parent, no matter what is
>  * configured. Otherwise we won't commit any
>  * submodules which were manually staged, which would
>  * be really confusing.
>  */
> int diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
>
> in prepare_commit, so commit ignores the .gitmodules file.
>
> This allows git-add to add ignored submodules, currently ignored submodules
> would have to be added using the plumbing
> git update-index --add --cacheinfo 16,$SHA1,

Let me play devil's advocate (as I have this suspicion that .ignore
thing specific for submodule is probably misdesigned and certainly
its implementation is backwards).  Is the primary use case for this
.ignore thing to be able to do

git add .

without having to worry about adding the submodule marked as such?  
And if so, wouldn't it surprise these users who do use .ignore if
"git add" suddenly started adding them?

I think the right tool to use these days for excluding some paths
when adding all others is the negative pathspec; perhaps back when
the .ignore thing was added, it didn't exist or not widely known?  

I suspect that it may result in a better system overall if we can
deprecate and remove the submodule-specific .ignore thing.  At
least, I think the DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG is backwards
in that .ignore causes a submodule to be excluded from the diff by
default and forces paths that care about differences to opt into the
"override" thing, which is wrong---the specific UI thing that wants
not to show them should instead opt into ignoring, while keeping the
default not to special case such a flag that can only be set to a
submodule path.

> This makes sense, though a test demonstrating the change in behavior
> would be nice, but git-add doesn't seem to change as it doesn't even load
> the git modules config?
>
>> rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
>> run_diff_files(, DIFF_RACY_IS_MODIFIED);
>> return !!data.add_errors;
>> diff --git a/builtin/reset.c b/builtin/reset.c
>> index 046403ed6..772d078b8 100644
>> --- a/builtin/reset.c
>> +++ b/builtin/reset.c
>> @@ -156,6 +156,7 @@ static int read_from_tree(const struct pathspec 
>> *pathspec,
>> opt.output_format = DIFF_FORMAT_CALLBACK;
>> opt.format_callback = update_index_from_diff;
>> opt.format_callback_data = _to_add;
>> +   opt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
>
> same here? Also as this is not failing any test, it may be worth adding one
> to document the behavior of the "submodule..ignore" flag in tests?
>
>>
>> if (do_diff_cache(tree_oid, ))
>> return 1;
>> --
>> 2.14.0.rc0.400.g1c36432dff-goog
>>

Re: [PATCH] submodule: correct error message for missing commits.

[Junio C Hamano]

Stefan Beller  writes:

> We _do_ show the submodule as demonstrated by the code sample above
> if we possess the objects.
> ...
>> That sounds like a very sensible and user-centric behaviour to me,
>> and "not initialized" sounds like the right message to give in such
>> a case (as opposed to "commits not present"---even the user told us
>> they are not interesting, we may have them, so "not present" is not
>> just incorrect but irrelevant because that is not the reason why we
>> are not showing).
>
> So you are saying we should instead do:
>
>   if (not active)
> message = "not initialized"
>   if (problems with object loading)
> message = "commits not present"
>   ...

I think I am.

>> Or are you saying that even the user told us that the submodule is
>> not interesting, if we had "init" it earlier even once, we show the
>> difference and with a wrong label?  Showing the difference sounds
>> like a bug that is more severe than using a wrong label to me.
>
> I looked through the man pages and they never specify if submodule
> activeness affects the superproject diff, so we'd want to change that
> so that only active submodules are diffed.

I would think that would match my expectation more closely; if I
explicitly told Git to "deinit", and still see the diff to distrat
me (i.e. the current behaviour), I would probably feel that it is a
bug.  I do not know about others who are used to the current
beehaviour, though.  Do people actively "deinit" a submodule that
they once "init"ed, and if so for what purpose?  It's not like they
want to release the disk resource, so I'd imagine the only reason is
to reduce the distracting noise, but I'd prefer to hear from real
users rather than speculating.

Thanks.

Re: [PATCH 02/15] submodule: don't use submodule_from_name

[Junio C Hamano]

Stefan Beller  writes:

> Rereading the archives, there was quite some discussion on the design
> of these patches, but these lines of code did not get any attention
>
> https://public-inbox.org/git/4cdb3063.5010...@web.de/
>
> I cc'd Jens in the hope of him having a good memory why he
> wrote the code that way. :)

Thanks for digging.  I wouldn't be surprised if this were a fallback
to help a broken entry in .gitmodules that lack .path variable, but
we shouldn't be sweeping the problem under the rug like that.  

I wonder if we should barf loudly if there shouldn't be a submodule
at that path, i.e.

if (!submodule)
die("there is no submodule defined for path '%s'"...);

though.

> Note that this is the last caller of submodule_from_name being
> removed, so I would expect removal of submodule_from_name from
> the t/helper/test-submodule-config.c as well as
> Documentation/technical/api-submodule-config.txt
> in a later part of this series. (Well technically it could go outside
> of the series, but in the mean time we'd document and test
> dead code)

Good thinking.  As this is "cleanup" series, I think it is within
its scope to remove an API function that becomes unused.

>
>> Signed-off-by: Brandon Williams 
>> ---
>>  submodule.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/submodule.c b/submodule.c
>> index 7e87e4698..fd391aea6 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -1177,8 +1177,6 @@ static int get_next_submodule(struct child_process *cp,
>> continue;
>>
>> submodule = submodule_from_path(_oid, ce->name);
>> -   if (!submodule)
>> -   submodule = submodule_from_name(_oid, ce->name);
>>
>> default_argv = "yes";
>> if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
>> --
>> 2.14.0.rc0.400.g1c36432dff-goog
>>

Re: [PATCH 01/15] t7411: check configuration parsing errors

[Junio C Hamano]

Brandon Williams  writes:

> Check for configuration parsing errors in '.gitmodules' in t7411, which
> is explicitly testing the submodule-config subsystem, instead of in
> t7400.  Also explicitly use the test helper instead of relying on the
> gitmodules file from being read in status.

Makes sense.

> ...
> - test_must_fail git status
> -'
> -...
> +test_expect_success 'configuration parsing with error' '
> + test_when_finished "rm -rf repo" &&
> + test_create_repo repo &&
> + cat >repo/.gitmodules <<-\EOF &&
> + [submodule "s"]
> + path
> + ignore
> + EOF
> + (
> + cd repo &&
> + test_must_fail test-submodule-config "" s 2>actual &&
> + test_i18ngrep "bad config" actual
> + )
> +'
> +
>  cat >super/expect <  Submodule name: 'a' for path 'a'
>  Submodule name: 'a' for path 'b'

Re: [PATCH] submodule: correct error message for missing commits.

[Stefan Beller]

On Wed, Jul 26, 2017 at 1:30 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> When a submodule diff should be displayed we currently just add the
>> submodule objects to the main object store and then e.g. walk the
>> revision graph and create a summary for that submodule.
>>
>> It is possible that we are missing the submodule either completely or
>> partially, which we currently differentiate with different error messages
>> depending on whether (1) the whole submodule object store is missing or
>> (2) just the needed for this particular diff. (1) is reported as
>> "not initialized", and (2) is reported as "commits not present".
>>
>> If a submodule is deinit'ed its repository data is still around inside
>> the superproject, such that the diff can still be produced. In that way
>> the error message (1) is misleading as we can have a diff despite the
>> submodule being not initialized.
>
> This is confusing...
>
> So are you saying that if we do "submodule init A && submodule
> update A" followed by "submodule deinit A",

  $ git clone https://gerrit.googlesource.com/gerrit
  $ git show --submodule=log bb798b00bb
  ...
  Submodule plugins/replication ... (not initialized)

  $ git submodule update --init
  $ # a good example of cross repo changes:
  $ git show --submodule=log bb798b00bb
  ...
  Submodule plugins/replication db4aecb2b8...98b7156cee:
  > Stop using WorkQueue#unregisterWorkQueue.
  < PushOne: Remove redundant string concatenation

  $ git submodule deinit -f --all
  $ git show --submodule=log bb798b00bb
  ...
  Submodule plugins/replication db4aecb2b8...98b7156cee:
  > Stop using WorkQueue#unregisterWorkQueue.
  < PushOne: Remove redundant string concatenation

  $ rm -rf .git/modules/*
  $ git show --submodule=log bb798b00bb
  ...
  Submodule plugins/replication ... (not initialized)

> we _could_ show the
> difference for submodule A between two commits in the superproject,
> because we already have the necessary data for the submodule, but we
> _choose_ not to show it because the user told us explicitly that the
> submodule is not interesting?

We _do_ show the submodule as demonstrated by the code sample above
if we possess the objects.

Hence the "not initialized" is not quite technically correct. (After deinit it
is not initialized, but we show nevertheless, so the user perceived
_reason_ why we do not show the submodule is "commits not present".

> That sounds like a very sensible and user-centric behaviour to me,
> and "not initialized" sounds like the right message to give in such
> a case (as opposed to "commits not present"---even the user told us
> they are not interesting, we may have them, so "not present" is not
> just incorrect but irrelevant because that is not the reason why we
> are not showing).

So you are saying we should instead do:

  if (not active)
message = "not initialized"
  if (problems with object loading)
message = "commits not present"
  ...

> Or are you saying that even the user told us that the submodule is
> not interesting, if we had "init" it earlier even once, we show the
> difference and with a wrong label?  Showing the difference sounds
> like a bug that is more severe than using a wrong label to me.

I looked through the man pages and they never specify if submodule
activeness affects the superproject diff, so we'd want to change that
so that only active submodules are diffed.

Re: [PATCH v2] contrib/rerere-train: optionally overwrite existing resolutions

[Junio C Hamano]

Raman Gupta  writes:

> On 26/07/17 02:05 PM, Junio C Hamano wrote:
>> I haven't tried this patch, but would this work well with options
>> meant for the 'git rev-list --parents "$@"' that grabs the list of
>> merge commits to learn from?  e.g.
>> 
>>  $ contrib/rerere-train.sh -n 4 --merges master
>>  $ contrib/rerere-train.sh --overwrite -n 4 --merges master
>>  $ contrib/rerere-train.sh -n 4 --overwrite --merges master
>> 
>> I do not think it is necessary to make the last one work; as long as
>> the first two work as expected, we are good even if the last one
>> dies with a sensible message e.g. "options X, Y and Z must be given
>> before other options" (currently "X, Y and Z" consists only of
>> "--overwrite", but I think you get what I mean).
>
> You're right -- I didn't try all the cases. I wasn't able to figure
> out how to get `rev-parse --parseopt` to deal with this situation, so
> I did it manually. I'm not super-happy with the result, but it does
> work. Look for PATCH v3.

Yes, I think you could squash the two case arms in the later loop
into one i.e.

-h|--help|-o|--overwrite)
die "please don't." ;;

but still the repetition does look ugly.

As a contrib/ material, I do not care too deeply about it, though.

Will queue.

Re: [PATCHv2] t8008: rely on rev-parse'd HEAD instead of sha1 value

[Junio C Hamano]

Stefan Beller  writes:

> Remove hard coded sha1 values, obtain the values using
> 'git rev-parse HEAD' which should be future proof regardless
> of the hash function used.
>
> Additionally future-proof the test by hard coding the
> abbreviation length of the hash.
>
> Signed-off-by: Stefan Beller 
> ---
>
>> Don't hardcoded lengths of the hashes defeat this future-proofing
>> effort, though?  It shouldn't be too hard to do the equivalent of
>> the auto computation of abbreviation in this script, which would be
>> true future-proofing, I guess.
>
> Added --abbrev=n to also hard code hash abbreviation. At first
> I had the impression of a off-by-one-error, but after reading the
> man page for both blame and rev-parse, I realize that blames
> abbrev notion is different than rev-parse precisely for the
> caret that may occur to indicate out-of-range.

OK, and 17 hexdigits ought to be sufficient for a sample repository
we create for the test---we shouldn't be picking a hash that would
cause hash prefix collisions with such a small number of objects and
reasonably long prefix anyway ;-)

Thanks, will queue.

>
>  t/t8008-blame-formats.sh | 30 --
>  1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/t/t8008-blame-formats.sh b/t/t8008-blame-formats.sh
> index 92c8e792d1..ae4b579d24 100755
> --- a/t/t8008-blame-formats.sh
> +++ b/t/t8008-blame-formats.sh
> @@ -12,22 +12,25 @@ test_expect_success 'setup' '
>   echo c >>file &&
>   echo d >>file &&
>   test_tick &&
> - git commit -a -m two
> + git commit -a -m two &&
> + ID1=$(git rev-parse HEAD^) &&
> + shortID1="^$(git rev-parse HEAD^ |cut -c 1-17)" &&
> + ID2=$(git rev-parse HEAD) &&
> + shortID2="$(git rev-parse HEAD |cut -c 1-18)"
>  '
>  
> -cat >expect <<'EOF'
> -^baf5e0b (A U Thor 2005-04-07 15:13:13 -0700 1) a
> -8825379d (A U Thor 2005-04-07 15:14:13 -0700 2) b
> -8825379d (A U Thor 2005-04-07 15:14:13 -0700 3) c
> -8825379d (A U Thor 2005-04-07 15:14:13 -0700 4) d
> +cat >expect < +$shortID1 (A U Thor 2005-04-07 15:13:13 -0700 1) a
> +$shortID2 (A U Thor 2005-04-07 15:14:13 -0700 2) b
> +$shortID2 (A U Thor 2005-04-07 15:14:13 -0700 3) c
> +$shortID2 (A U Thor 2005-04-07 15:14:13 -0700 4) d
>  EOF
>  test_expect_success 'normal blame output' '
> - git blame file >actual &&
> + git blame --abbrev=17 file >actual &&
>   test_cmp expect actual
>  '
>  
> -ID1=baf5e0b3869e0b2b2beb395a3720c7b51eac94fc
> -COMMIT1='author A U Thor
> +COMMIT1="author A U Thor
>  author-mail 
>  author-time 1112911993
>  author-tz -0700
> @@ -37,9 +40,8 @@ committer-time 1112911993
>  committer-tz -0700
>  summary one
>  boundary
> -filename file'
> -ID2=8825379dfb8a1267b58e8e5bcf69eec838f685ec
> -COMMIT2='author A U Thor
> +filename file"
> +COMMIT2="author A U Thor
>  author-mail 
>  author-time 1112912053
>  author-tz -0700
> @@ -48,8 +50,8 @@ committer-mail 
>  committer-time 1112912053
>  committer-tz -0700
>  summary two
> -previous baf5e0b3869e0b2b2beb395a3720c7b51eac94fc file
> -filename file'
> +previous $ID1 file
> +filename file"
>  
>  cat >expect <  $ID1 1 1 1

Re: [PATCH] submodule: correct error message for missing commits.

[Junio C Hamano]

Stefan Beller  writes:

> When a submodule diff should be displayed we currently just add the
> submodule objects to the main object store and then e.g. walk the
> revision graph and create a summary for that submodule.
>
> It is possible that we are missing the submodule either completely or
> partially, which we currently differentiate with different error messages
> depending on whether (1) the whole submodule object store is missing or
> (2) just the needed for this particular diff. (1) is reported as
> "not initialized", and (2) is reported as "commits not present".
>
> If a submodule is deinit'ed its repository data is still around inside
> the superproject, such that the diff can still be produced. In that way
> the error message (1) is misleading as we can have a diff despite the
> submodule being not initialized.

This is confusing...  

So are you saying that if we do "submodule init A && submodule
update A" followed by "submodule deinit A", we _could_ show the
difference for submodule A between two commits in the superproject,
because we already have the necessary data for the submodule, but we
_choose_ not to show it because the user told us explicitly that the
submodule is not interesting?

That sounds like a very sensible and user-centric behaviour to me,
and "not initialized" sounds like the right message to give in such
a case (as opposed to "commits not present"---even the user told us
they are not interesting, we may have them, so "not present" is not
just incorrect but irrelevant because that is not the reason why we
are not showing).

Or are you saying that even the user told us that the submodule is
not interesting, if we had "init" it earlier even once, we show the
difference and with a wrong label?  Showing the difference sounds
like a bug that is more severe than using a wrong label to me.

Puzzled.

>
> Downgrade the error message (1) to be the same as (2) and just say
> the commits are not present, as that is the true reason why the diff
> cannot be shown.
>
> Signed-off-by: Stefan Beller 
> ---
>
> I came across this error message in the series for the
> object store modularisation[1], when I was trying to replace
> 'add_submodule_odb' by a custom loaded object store from a
> submodule repo object, which got me thinking on the error
> message and the true cause for it.  
>
> While this could go in separately, I may carry it in that
> series, as there we'd come up with more error messages
> ("could not create submodule object store" as well as the
> "commits not present", maybe even "submodule not lookup failed")
>
> Thanks,
> Stefan
>
> [1] https://public-inbox.org/git/20170706202739.6056-1-sbel...@google.com/
>   
>
>  submodule.c   | 2 +-
>  t/t4059-diff-submodule-not-initialized.sh | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 6531c5d609..280c246477 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -567,7 +567,7 @@ static void show_submodule_header(FILE *f, const char 
> *path,
>  
>   if (add_submodule_odb(path)) {
>   if (!message)
> - message = "(not initialized)";
> + message = "(commits not present)";
>   goto output_header;
>   }
>  
> diff --git a/t/t4059-diff-submodule-not-initialized.sh 
> b/t/t4059-diff-submodule-not-initialized.sh
> index cd70fd5192..49bca7b48d 100755
> --- a/t/t4059-diff-submodule-not-initialized.sh
> +++ b/t/t4059-diff-submodule-not-initialized.sh
> @@ -95,7 +95,7 @@ test_expect_success 'submodule not initialized in new 
> clone' '
>   git clone . sm3 &&
>   git -C sm3 diff-tree -p --no-commit-id --submodule=log HEAD >actual &&
>   cat >expected <<-EOF &&
> - Submodule sm1 $smhead1...$smhead2 (not initialized)
> + Submodule sm1 $smhead1...$smhead2 (commits not present)
>   EOF
>   test_cmp expected actual
>  '

Re: [PATCH/RFC] setup: update error message to be more meaningful

[Junio C Hamano]

Jonathan Nieder  writes:

> For an initial guess: in the example
>
>   git grep test -n
>
> ...
>  2. Focus on "argument" instead of "filename" so that the message
> could still apply: something like
>
>   fatal: option '-n' must come before non-option arguments

I think this one is the most sensible.  There may or may not be a
file called "test" in the working tree, and the user may or may not
meant to look for a pattern "test".  What is wrong in the sample
command line is that "test" is not a dashed option and yet it has a
dashed option "-n" after it, and your version clearly explains it.

[PATCH] submodule: correct error message for missing commits.

[Stefan Beller]

When a submodule diff should be displayed we currently just add the
submodule objects to the main object store and then e.g. walk the
revision graph and create a summary for that submodule.

It is possible that we are missing the submodule either completely or
partially, which we currently differentiate with different error messages
depending on whether (1) the whole submodule object store is missing or
(2) just the needed for this particular diff. (1) is reported as
"not initialized", and (2) is reported as "commits not present".

If a submodule is deinit'ed its repository data is still around inside
the superproject, such that the diff can still be produced. In that way
the error message (1) is misleading as we can have a diff despite the
submodule being not initialized.

Downgrade the error message (1) to be the same as (2) and just say
the commits are not present, as that is the true reason why the diff
cannot be shown.

Signed-off-by: Stefan Beller 
---

I came across this error message in the series for the
object store modularisation[1], when I was trying to replace
'add_submodule_odb' by a custom loaded object store from a
submodule repo object, which got me thinking on the error
message and the true cause for it.  

While this could go in separately, I may carry it in that
series, as there we'd come up with more error messages
("could not create submodule object store" as well as the
"commits not present", maybe even "submodule not lookup failed")

Thanks,
Stefan

[1] https://public-inbox.org/git/20170706202739.6056-1-sbel...@google.com/
  

 submodule.c   | 2 +-
 t/t4059-diff-submodule-not-initialized.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 6531c5d609..280c246477 100644
--- a/submodule.c
+++ b/submodule.c
@@ -567,7 +567,7 @@ static void show_submodule_header(FILE *f, const char *path,
 
if (add_submodule_odb(path)) {
if (!message)
-   message = "(not initialized)";
+   message = "(commits not present)";
goto output_header;
}
 
diff --git a/t/t4059-diff-submodule-not-initialized.sh 
b/t/t4059-diff-submodule-not-initialized.sh
index cd70fd5192..49bca7b48d 100755
--- a/t/t4059-diff-submodule-not-initialized.sh
+++ b/t/t4059-diff-submodule-not-initialized.sh
@@ -95,7 +95,7 @@ test_expect_success 'submodule not initialized in new clone' '
git clone . sm3 &&
git -C sm3 diff-tree -p --no-commit-id --submodule=log HEAD >actual &&
cat >expected <<-EOF &&
-   Submodule sm1 $smhead1...$smhead2 (not initialized)
+   Submodule sm1 $smhead1...$smhead2 (commits not present)
EOF
test_cmp expected actual
 '
-- 
2.14.0.rc0.3.g6c2e499285

Re: [PATCH for NEXT v3 0/2] sub-process: refactor handshake to common function

[Junio C Hamano]

Jonathan Tan  writes:

> Thanks for all your comments.
>
> This is now built off "next" to include Lars's changes.
>
> About whether this is too restrictive (for example, as Junio mentions,
> this will not allow future capabilities of the form "key=???"), I think
> that this can be upgraded later if necessary. For now, both the filter
> code and the ODB (and ODB-like) proposals on the mailing list do not
> require such a thing, so I have not included that functionality.

Heh, anything can be updated later.  The real question is how much
damage such an update may incur.  If such an update ends up having
to dismantle large part of what you did, then that is not a real
consolation that it "can be" updated later.

I think subprocess_handshake() can become the lowest level helper
that takes callback function to act on the capability request that
was received, and the existing user like start_multi_file_filter_fn()
can be served by a thin-wrapper around subprocess_handshake() that
defines a standard/built-in callback that checks the capability list
for exact match and flips the bit in an unsigned, so it is possible
to limit the damage when such an enhancement happens.



>
> Changes from v2:
>  - now rebased onto next, to pick up Lars's changes
>  - split up into more functions
>  - welcome prefix does not include final dash
>  - no more gotos in expected cases (or at all)
>
> Jonathan Tan (2):
>   Documentation: migrate sub-process docs to header
>   sub-process: refactor handshake to common function
>
>  Documentation/technical/api-sub-process.txt |  59 
>  convert.c   |  75 ++--
>  pkt-line.c  |  19 -
>  pkt-line.h  |   2 -
>  sub-process.c   | 103 
> 
>  sub-process.h   |  51 +-
>  t/t0021-conversion.sh   |   2 +-
>  7 files changed, 160 insertions(+), 151 deletions(-)
>  delete mode 100644 Documentation/technical/api-sub-process.txt

Re: [PATCH] recursive submodules: detach HEAD from new state

[Junio C Hamano]

Stefan Beller  writes:

> So I took a step back and wrote about different proposals where
> we want to go long term. See below. This will help us
> figuring out how to approach this bug correctly.

Thanks for writing this.

> RFC: A new type of symbolic refs
>
> A symbolic ref can currently only point at a ref or another symbolic ref.
> This proposal show cases different scenarios on how this could change in
> the future.
>
>
>
> A: HEAD pointing at the superprojects index
> ===
>
> Introduce a new symbolic ref that points at the superprojects index of
> the gitlink. The format is
>
>   "repo:"  '\0'  '\0'
>
> Ref read operations
> ---
>   e.g. git log HEAD
>
> Just like existing symrefs, the content of the ref will be read and followed.
> On reading "repo:", the sha1 will be obtained equivalent to:
>
> git -C  ls-files -s  | awk '{ print $2}'
>
> In case of error
> (superproject not found, gitlink path does not exist), the ref is broken and
>
> Ref write operations driven by the submodule, affecting symrefs
> ---
>   e.g. git checkout  (in the submodule)
>
> In this scenario only the HEAD is optionally attached to the superproject,
> so we can rewrite the HEAD to be anything else, such as a branch just fine.
> Once the HEAD is not pointing at the superproject any more, we'll leave the
> submodule alone in operations driven by the superproject.

That explains what the proposed code _does_.  It does not explain
why the chosen behaviour is a sensible one.

This illustrates the point I have trouble with when trying to judge
all of these discrete update proposals to submodules.  They only say
"This feature does this in this case, does that in that case,..."
but lack "this is meant to be used when you want to implement the
workflow that goes like this, and fits as a building block at this
point for that workflow. Other elements needed to support that
workflow well are ...".  No proposal gives a big picture and explain
how these small bits fit together.

For example, I would understand better if this write-up of yours
were not organized with the "proposal X added A that behaves this
way and added B that behaves that way" as its major axis, but
instead was written with the workflow that is meant to be realized
as its major axis, e.g.

A project may want to use submodules as if it is just part of
superproject.  In such a project, checking out branch X at the
superproject level, working on files in both superproject and
submodules, and then committing recursively and pushing the
results out recursively at the superproject level, all would
want to affect the same branch X at all levels in the upstream.

may be one possible workflow you want to support.  As one ingredient
to support such structure, the HEAD in the submodule that points at
an index entry in the superproject may be very useful.  After a
recursive checkout at the superproject level, the HEAD of the
submodule ought to be what came from and recorded in the tree in the
superproject, and after a commit in the submodule, the HEAD moves to
the new commit and the entry in the superproject's index also gets
updated which would have a nice property that "commit" in submodule
acts almost like "add" in superproject.  A recursive "git diff" would
show that submodule is clean after such a commit, recursive "push"
would know which branch to push out, etc.

And when operating in such a mode, it would make most sense if "git
checkout" of a different branch Y in a submodule repository is
either forbidden, or should behave as if the submodule directory
were an ordinary directory of the superproject (i.e. causing
recursive checkout of the branch Y at the superproject level).

BUT.

Because none of the proposals paint a big picture (e.g. the big
picture the above hypothetical example gives is that the core
concept of this particular workflow being supported is that
everything recursively stays on the branch with the same name), we
cannot judge if it is sensible for "a new style symref" to be
updated/demoted to a normal branch pointer when "git checkout"
happens.  It is not sensible in such a hypothetical workflow, but it
may be very sensible in another workflow.  Without stating what
big-picture goal is being achieved, it is impossible to see if a
proposal to add/change an individual component that is to be used as
a building block makes sense.

Historically, we can get away without giving choices of "supported
workflows", allowing the user to pick one, and explaining how things
fit together, primarily because the operations that can recurse were
primarily read-only e.g. status, grep, etc., and the supported model
was "the user can be on whatever branch or detached in each
submodule that may or may not be consistent with what happens in the
superproject; it is up to the user to hang themselves with the long

[PATCH v3] contrib/rerere-train: optionally overwrite existing resolutions

[Raman Gupta]

Provide the user an option to overwrite existing resolutions using an
`--overwrite` flag. This might be used, for example, if the user knows
that they already have an entry in their rerere cache for a conflict,
but wish to drop it and retrain based on the merge commit(s) passed to
the rerere-train script.

Signed-off-by: Raman Gupta 
---
 contrib/rerere-train.sh | 54 +++--
 1 file changed, 52 insertions(+), 2 deletions(-)

diff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh
index 52ad9e4..45d 100755
--- a/contrib/rerere-train.sh
+++ b/contrib/rerere-train.sh
@@ -3,10 +3,56 @@
 # Prime rerere database from existing merge commits
 
 me=rerere-train
-USAGE="$me rev-list-args"
+USAGE=$(cat <<-EOF
+usage: $me [--overwrite] 
+
+-h, --helpshow the help
+-o, --overwrite   overwrite any existing rerere cache
+EOF
+)
 
 SUBDIRECTORY_OK=Yes
-OPTIONS_SPEC=
+
+overwrite=0
+
+while test $# -gt 0
+do
+   opt="$1"
+   case "$opt" in
+   -h|--help)
+   echo "$USAGE"
+   exit 0
+   ;;
+   -o|--overwrite)
+   overwrite=1
+   shift
+   break
+   ;;
+   --)
+   shift
+   break
+   ;;
+   *)
+   break
+   ;;
+   esac
+done
+
+# Overwrite or help options are not valid except as first arg
+for opt in "$@"
+do
+   case "$opt" in
+   -h|--help)
+   echo "$USAGE"
+   exit 0
+   ;;
+   -o|--overwrite)
+   echo "$USAGE"
+   exit 0
+   ;;
+   esac
+done
+
 . "$(git --exec-path)/git-sh-setup"
 require_work_tree
 cd_to_toplevel
@@ -34,6 +80,10 @@ do
# Cleanly merges
continue
fi
+   if test $overwrite = 1
+   then
+   git rerere forget .
+   fi
if test -s "$GIT_DIR/MERGE_RR"
then
git show -s --pretty=format:"Learning from %h %s" "$commit"
-- 
2.9.4

[PATCHv2] t8008: rely on rev-parse'd HEAD instead of sha1 value

[Stefan Beller]

Remove hard coded sha1 values, obtain the values using
'git rev-parse HEAD' which should be future proof regardless
of the hash function used.

Additionally future-proof the test by hard coding the
abbreviation length of the hash.

Signed-off-by: Stefan Beller 
---

> Don't hardcoded lengths of the hashes defeat this future-proofing
> effort, though?  It shouldn't be too hard to do the equivalent of
> the auto computation of abbreviation in this script, which would be
> true future-proofing, I guess.

Added --abbrev=n to also hard code hash abbreviation. At first
I had the impression of a off-by-one-error, but after reading the
man page for both blame and rev-parse, I realize that blames
abbrev notion is different than rev-parse precisely for the
caret that may occur to indicate out-of-range.

 t/t8008-blame-formats.sh | 30 --
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/t/t8008-blame-formats.sh b/t/t8008-blame-formats.sh
index 92c8e792d1..ae4b579d24 100755
--- a/t/t8008-blame-formats.sh
+++ b/t/t8008-blame-formats.sh
@@ -12,22 +12,25 @@ test_expect_success 'setup' '
echo c >>file &&
echo d >>file &&
test_tick &&
-   git commit -a -m two
+   git commit -a -m two &&
+   ID1=$(git rev-parse HEAD^) &&
+   shortID1="^$(git rev-parse HEAD^ |cut -c 1-17)" &&
+   ID2=$(git rev-parse HEAD) &&
+   shortID2="$(git rev-parse HEAD |cut -c 1-18)"
 '
 
-cat >expect <<'EOF'
-^baf5e0b (A U Thor 2005-04-07 15:13:13 -0700 1) a
-8825379d (A U Thor 2005-04-07 15:14:13 -0700 2) b
-8825379d (A U Thor 2005-04-07 15:14:13 -0700 3) c
-8825379d (A U Thor 2005-04-07 15:14:13 -0700 4) d
+cat >expect actual &&
test_cmp expect actual
 '
 
-ID1=baf5e0b3869e0b2b2beb395a3720c7b51eac94fc
-COMMIT1='author A U Thor
+COMMIT1="author A U Thor
 author-mail 
 author-time 1112911993
 author-tz -0700
@@ -37,9 +40,8 @@ committer-time 1112911993
 committer-tz -0700
 summary one
 boundary
-filename file'
-ID2=8825379dfb8a1267b58e8e5bcf69eec838f685ec
-COMMIT2='author A U Thor
+filename file"
+COMMIT2="author A U Thor
 author-mail 
 author-time 1112912053
 author-tz -0700
@@ -48,8 +50,8 @@ committer-mail 
 committer-time 1112912053
 committer-tz -0700
 summary two
-previous baf5e0b3869e0b2b2beb395a3720c7b51eac94fc file
-filename file'
+previous $ID1 file
+filename file"
 
 cat >expect <

Re: [PATCH v2] contrib/rerere-train: optionally overwrite existing resolutions

[Raman Gupta]

On 26/07/17 02:05 PM, Junio C Hamano wrote:
> I haven't tried this patch, but would this work well with options
> meant for the 'git rev-list --parents "$@"' that grabs the list of
> merge commits to learn from?  e.g.
> 
>   $ contrib/rerere-train.sh -n 4 --merges master
>   $ contrib/rerere-train.sh --overwrite -n 4 --merges master
>   $ contrib/rerere-train.sh -n 4 --overwrite --merges master
> 
> I do not think it is necessary to make the last one work; as long as
> the first two work as expected, we are good even if the last one
> dies with a sensible message e.g. "options X, Y and Z must be given
> before other options" (currently "X, Y and Z" consists only of
> "--overwrite", but I think you get what I mean).

You're right -- I didn't try all the cases. I wasn't able to figure
out how to get `rev-parse --parseopt` to deal with this situation, so
I did it manually. I'm not super-happy with the result, but it does
work. Look for PATCH v3.

Regards,
Raman

Re: What's cooking in git.git (Jul 2017, #07; Mon, 24)

[Junio C Hamano]

Johannes Schindelin  writes:

>> * wd/rebase-conflict-guide (2017-07-17) 1 commit
>>   (merged to 'next' on 2017-07-20 at c78e758b23)
>>  + rebase: make resolve message clearer for inexperienced users
>> 
>>  Code clean-up.
>
> This is not a code clean-up. It is an improvement of the user experience.

Indeed it is.  Thanks for spotting.

Re: [RFC PATCH 0/5] Add option to autostage changes when continuing a rebase

[Junio C Hamano]

Phillip Wood  writes:

> From: Phillip Wood 
>
> These patches add an '--autostage' option (and corresponding config
> variable) to 'rebase --continue' that will stage any unstaged changes
> before continuing. This saves the user having to type 'git add' before
> running 'git rebase --continue'.

I wonder if this interacts with existing rerere.autoupdate
configuration variable and if so how (i.e. would they conflict and
fight with each other?  would they work well together?  would one of
them make the other unnecessary?).  In any case, they look closely
related and perhaps should be named similarly.

I even have a suspicion that they may be essentially doing the same
thing.

For a previous discussion, you start from here:

  https://public-inbox.org/git/7vej6xb4lr@gitster.siamese.dyndns.org/#t

and for the context, look at the original post by Ingo, to which the
above message is a response to.

Thanks.

>
> Phillip Wood (5):
>   rebase --continue: add --autostage to stage unstaged changes
>   rebase -i: improve --continue --autostage
>   Unify rebase amend message when HEAD has changed
>   Add tests for rebase --continue --autostage
>   Add rebase.continue.autostage config setting
>
>  git-rebase--am.sh |   1 +
>  git-rebase--interactive.sh| 111 
> --
>  git-rebase--merge.sh  |   1 +
>  git-rebase.sh |  76 ++---
>  sequencer.c   |  22 +++--
>  t/t3404-rebase-interactive.sh |   2 +-
>  t/t3418-rebase-continue.sh|  50 ++-
>  7 files changed, 222 insertions(+), 41 deletions(-)

[PATCH for NEXT v3 0/2] sub-process: refactor handshake to common function

[Jonathan Tan]

Thanks for all your comments.

This is now built off "next" to include Lars's changes.

About whether this is too restrictive (for example, as Junio mentions,
this will not allow future capabilities of the form "key=???"), I think
that this can be upgraded later if necessary. For now, both the filter
code and the ODB (and ODB-like) proposals on the mailing list do not
require such a thing, so I have not included that functionality.

Changes from v2:
 - now rebased onto next, to pick up Lars's changes
 - split up into more functions
 - welcome prefix does not include final dash
 - no more gotos in expected cases (or at all)

Jonathan Tan (2):
  Documentation: migrate sub-process docs to header
  sub-process: refactor handshake to common function

 Documentation/technical/api-sub-process.txt |  59 
 convert.c   |  75 ++--
 pkt-line.c  |  19 -
 pkt-line.h  |   2 -
 sub-process.c   | 103 
 sub-process.h   |  51 +-
 t/t0021-conversion.sh   |   2 +-
 7 files changed, 160 insertions(+), 151 deletions(-)
 delete mode 100644 Documentation/technical/api-sub-process.txt

-- 
2.14.0.rc0.400.g1c36432dff-goog

[PATCH for NEXT v3 2/2] sub-process: refactor handshake to common function

[Jonathan Tan]

Refactor, into a common function, the version and capability negotiation
done when invoking a long-running process as a clean or smudge filter.
This will be useful for other Git code that needs to interact similarly
with a long-running process.

As you can see in the change to t0021, this commit changes the error
message reported when the long-running process does not introduce itself
with the expected "server"-terminated line. Originally, the error
message reports that the filter "does not support filter protocol
version 2", differentiating between the old single-file filter protocol
and the new multi-file filter protocol - I have updated it to something
more generic and useful.

Signed-off-by: Jonathan Tan 
---
 convert.c |  75 
 pkt-line.c|  19 --
 pkt-line.h|   2 -
 sub-process.c | 103 ++
 sub-process.h |  26 +
 t/t0021-conversion.sh |   2 +-
 6 files changed, 137 insertions(+), 90 deletions(-)

diff --git a/convert.c b/convert.c
index dbdbb24e4..1012462e3 100644
--- a/convert.c
+++ b/convert.c
@@ -513,78 +513,17 @@ static struct hashmap subprocess_map;
 
 static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)
 {
-   int err, i;
-   struct cmd2process *entry = (struct cmd2process *)subprocess;
-   struct string_list cap_list = STRING_LIST_INIT_NODUP;
-   char *cap_buf;
-   const char *cap_name;
-   struct child_process *process = >process;
-   const char *cmd = subprocess->cmd;
-
-   static const struct {
-   const char *name;
-   unsigned int cap;
-   } known_caps[] = {
+   static int versions[] = {2, 0};
+   static struct subprocess_capability capabilities[] = {
{ "clean",  CAP_CLEAN  },
{ "smudge", CAP_SMUDGE },
{ "delay",  CAP_DELAY  },
+   { NULL, 0 }
};
-
-   sigchain_push(SIGPIPE, SIG_IGN);
-
-   err = packet_writel(process->in, "git-filter-client", "version=2", 
NULL);
-   if (err)
-   goto done;
-
-   err = strcmp(packet_read_line(process->out, NULL), "git-filter-server");
-   if (err) {
-   error("external filter '%s' does not support filter protocol 
version 2", cmd);
-   goto done;
-   }
-   err = strcmp(packet_read_line(process->out, NULL), "version=2");
-   if (err)
-   goto done;
-   err = packet_read_line(process->out, NULL) != NULL;
-   if (err)
-   goto done;
-
-   for (i = 0; i < ARRAY_SIZE(known_caps); ++i) {
-   err = packet_write_fmt_gently(
-   process->in, "capability=%s\n", known_caps[i].name);
-   if (err)
-   goto done;
-   }
-   err = packet_flush_gently(process->in);
-   if (err)
-   goto done;
-
-   for (;;) {
-   cap_buf = packet_read_line(process->out, NULL);
-   if (!cap_buf)
-   break;
-   string_list_split_in_place(_list, cap_buf, '=', 1);
-
-   if (cap_list.nr != 2 || strcmp(cap_list.items[0].string, 
"capability"))
-   continue;
-
-   cap_name = cap_list.items[1].string;
-   i = ARRAY_SIZE(known_caps) - 1;
-   while (i >= 0 && strcmp(cap_name, known_caps[i].name))
-   i--;
-
-   if (i >= 0)
-   entry->supported_capabilities |= known_caps[i].cap;
-   else
-   warning("external filter '%s' requested unsupported 
filter capability '%s'",
-   cmd, cap_name);
-
-   string_list_clear(_list, 0);
-   }
-
-done:
-   sigchain_pop(SIGPIPE);
-
-   return err;
+   struct cmd2process *entry = (struct cmd2process *)subprocess;
+   return subprocess_handshake(subprocess, "git-filter", versions, NULL,
+   capabilities,
+   >supported_capabilities);
 }
 
 static void handle_filter_error(const struct strbuf *filter_status,
diff --git a/pkt-line.c b/pkt-line.c
index 9d845ecc3..7db911957 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -171,25 +171,6 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
return status;
 }
 
-int packet_writel(int fd, const char *line, ...)
-{
-   va_list args;
-   int err;
-   va_start(args, line);
-   for (;;) {
-   if (!line)
-   break;
-   if (strlen(line) > LARGE_PACKET_DATA_MAX)
-   return -1;
-   err = packet_write_fmt_gently(fd, "%s\n", line);
-   if (err)
-   return err;
-   line = va_arg(args, const char*);
-   }
-   va_end(args);
-   return 

[PATCH for NEXT v3 1/2] Documentation: migrate sub-process docs to header

[Jonathan Tan]

Move the documentation for the sub-process API from a separate txt file
to its header file.

Signed-off-by: Jonathan Tan 
---
 Documentation/technical/api-sub-process.txt | 59 -
 sub-process.h   | 25 +++-
 2 files changed, 23 insertions(+), 61 deletions(-)
 delete mode 100644 Documentation/technical/api-sub-process.txt

diff --git a/Documentation/technical/api-sub-process.txt 
b/Documentation/technical/api-sub-process.txt
deleted file mode 100644
index 793508cf3..0
--- a/Documentation/technical/api-sub-process.txt
+++ /dev/null
@@ -1,59 +0,0 @@
-sub-process API
-===
-
-The sub-process API makes it possible to run background sub-processes
-for the entire lifetime of a Git invocation. If Git needs to communicate
-with an external process multiple times, then this can reduces the process
-invocation overhead. Git and the sub-process communicate through stdin and
-stdout.
-
-The sub-processes are kept in a hashmap by command name and looked up
-via the subprocess_find_entry function.  If an existing instance can not
-be found then a new process should be created and started.  When the
-parent git command terminates, all sub-processes are also terminated.
-
-This API is based on the run-command API.
-
-Data structures

-
-* `struct subprocess_entry`
-
-The sub-process structure.  Members should not be accessed directly.
-
-Types
--
-
-'int(*subprocess_start_fn)(struct subprocess_entry *entry)'::
-
-   User-supplied function to initialize the sub-process.  This is
-   typically used to negotiate the interface version and capabilities.
-
-
-Functions
--
-
-`cmd2process_cmp`::
-
-   Function to test two subprocess hashmap entries for equality.
-
-`subprocess_start`::
-
-   Start a subprocess and add it to the subprocess hashmap.
-
-`subprocess_stop`::
-
-   Kill a subprocess and remove it from the subprocess hashmap.
-
-`subprocess_find_entry`::
-
-   Find a subprocess in the subprocess hashmap.
-
-`subprocess_get_child_process`::
-
-   Get the underlying `struct child_process` from a subprocess.
-
-`subprocess_read_status`::
-
-   Helper function to read packets looking for the last "status="
-   key/value pair.
diff --git a/sub-process.h b/sub-process.h
index 8cd07a59a..d37c1499a 100644
--- a/sub-process.h
+++ b/sub-process.h
@@ -6,12 +6,23 @@
 #include "run-command.h"
 
 /*
- * Generic implementation of background process infrastructure.
- * See: Documentation/technical/api-sub-process.txt
+ * The sub-process API makes it possible to run background sub-processes
+ * for the entire lifetime of a Git invocation. If Git needs to communicate
+ * with an external process multiple times, then this can reduces the process
+ * invocation overhead. Git and the sub-process communicate through stdin and
+ * stdout.
+ *
+ * The sub-processes are kept in a hashmap by command name and looked up
+ * via the subprocess_find_entry function.  If an existing instance can not
+ * be found then a new process should be created and started.  When the
+ * parent git command terminates, all sub-processes are also terminated.
+ * 
+ * This API is based on the run-command API.
  */
 
  /* data structures */
 
+/* Members should not be accessed directly. */
 struct subprocess_entry {
struct hashmap_entry ent; /* must be the first member! */
const char *cmd;
@@ -20,21 +31,31 @@ struct subprocess_entry {
 
 /* subprocess functions */
 
+/* Function to test two subprocess hashmap entries for equality. */
 extern int cmd2process_cmp(const void *unused_cmp_data,
   const void *e1,
   const void *e2,
   const void *unused_keydata);
 
+/*
+ * User-supplied function to initialize the sub-process.  This is
+ * typically used to negotiate the interface version and capabilities.
+ */
 typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);
+
+/* Start a subprocess and add it to the subprocess hashmap. */
 int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, 
const char *cmd,
subprocess_start_fn startfn);
 
+/* Kill a subprocess and remove it from the subprocess hashmap. */
 void subprocess_stop(struct hashmap *hashmap, struct subprocess_entry *entry);
 
+/* Find a subprocess in the subprocess hashmap. */
 struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, const 
char *cmd);
 
 /* subprocess helper functions */
 
+/* Get the underlying `struct child_process` from a subprocess. */
 static inline struct child_process *subprocess_get_child_process(
struct subprocess_entry *entry)
 {
-- 
2.14.0.rc0.400.g1c36432dff-goog

Re: [PATCH] sub-process: refactor handshake to common function

[Junio C Hamano]

Lars Schneider  writes:

> Please note that I've recently refactored the capabilities negotiation a bit:
> https://github.com/git/git/commit/1514c8edd62d96006cd1de31e906ed5798dd4681
>
> This change is still cooking in `next`. I am not sure how this should/could
> be handled but maybe you can use my refactoring as your base?

I think they can play well together as independent topics.  The
known_caps[] thing you introduced essentially is the same as the
struct subprocess_capability capablities[] Jonathan has.

Please check

$ git show "pu^{/jt/subprocess-handshake' into pu}" convert.c

to see if it makes sense.

Thanks.

Re: Reducing redundant build at Travis?

[Junio C Hamano]

Lars Schneider  writes:

> ... I started 
> to work on a patch that moves all TravisCI logic into scripts located 
> in the `ci` folder. These scripts share a `lib-travisci.sh` for common 
> functions such as `skip_branch_tip_with_tag ()` executed at the
> beginning of every script.
>
> Does this sound sensible to you? I am a bit busy with non Git related 
> work right now but I try to post the patch for you to review ASAP.

Absolutely fantastic.

Take your time; there is no need to rush and haste makes waste.

Thanks for working on this.

Re: [PATCH v2] contrib/rerere-train: optionally overwrite existing resolutions

[Junio C Hamano]

Raman Gupta  writes:

> Provide the user an option to overwrite existing resolutions using an
> `--overwrite` flag. This might be used, for example, if the user knows
> that they already have an entry in their rerere cache for a conflict,
> but wish to drop it and retrain based on the merge commit(s) passed to
> the rerere-train script.
>
> Signed-off-by: Raman Gupta 
> ---
>  contrib/rerere-train.sh | 36 ++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh
> index 52ad9e4..e25bf8a 100755
> --- a/contrib/rerere-train.sh
> +++ b/contrib/rerere-train.sh
> @@ -3,10 +3,38 @@
>  # Prime rerere database from existing merge commits
>  
>  me=rerere-train
> -USAGE="$me rev-list-args"
>  
>  SUBDIRECTORY_OK=Yes
> -OPTIONS_SPEC=
> +OPTS_SPEC="\
> +$me [--overwrite] 
> +--
> +h,helpshow the help
> +o,overwrite   overwrite any existing rerere cache
> +"
> +eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit 
> $?)"
> +
> +overwrite=0

It is very good that you initialize overwrite explicitly here, to
prevent it from seeping through from the caller's environment.

> +while test $# -gt 0
> +do
> + opt="$1"
> + case "$opt" in
> + -o)
> + overwrite=1
> + shift
> + shift
> + ;;
> + --)
> + shift
> + break
> + ;;
> + *)
> + break
> + exit 1
> + ;;
> + esac
> +done

I haven't tried this patch, but would this work well with options
meant for the 'git rev-list --parents "$@"' that grabs the list of
merge commits to learn from?  e.g.

$ contrib/rerere-train.sh -n 4 --merges master
$ contrib/rerere-train.sh --overwrite -n 4 --merges master
$ contrib/rerere-train.sh -n 4 --overwrite --merges master

I do not think it is necessary to make the last one work; as long as
the first two work as expected, we are good even if the last one
dies with a sensible message e.g. "options X, Y and Z must be given
before other options" (currently "X, Y and Z" consists only of
"--overwrite", but I think you get what I mean).

>  . "$(git --exec-path)/git-sh-setup"
>  require_work_tree
>  cd_to_toplevel
> @@ -34,6 +62,10 @@ do
>   # Cleanly merges
>   continue
>   fi
> + if [ $overwrite == 1 ]

if test "$overwrite" = 1

cf. Documentation/CodingGuidelines.

> + then
> + git rerere forget .
> + fi
>   if test -s "$GIT_DIR/MERGE_RR"
>   then
>   git show -s --pretty=format:"Learning from %h %s" "$commit"

Re: [PATCH v1 0/4] Teach 'run' perf script to read config files

[Jeff King]

On Wed, Jul 26, 2017 at 05:58:09PM +0200, Christian Couder wrote:

> Actually after taking another look at that, it looks like the following 
> happens:
> 
> 1) the run script sources the original GIT-BUILD-OPTIONS file from
> ../.. relative to its location
> 2) a git version is built in "build/$rev" using GIT_PERF_MAKE_OPTS
> which generates a new GIT-BUILD-OPTIONS file in "build/$rev/"
> 3) when the actual perf scripts are run they source the original
> GIT-BUILD-OPTIONS file (through perf-lib.sh which sources test-lib.sh)

Right, the perf scripts are run in the context of the "outer"
repository, and get their options from that one. I think that's
intentional, and does the right thing for GIT_PERF_* options. It's
possibly confusing if the tests really do want to know about the build
options for a particular test-build (like asking if it was built with
NO_PERL, for example).

I think in practice it works out OK, because we tend to do test-builds
that are similar to what's in the outer repo (because we copy
config.mak, and don't tend to add a lot of command-line options). But if
you put exotic options into your GIT_PERF_MAKE_OPTS, they won't be
reflected in the config that the test scripts see.

> I wonder how useful 1) is, as the variables sourced from original
> GIT-BUILD-OPTIONS are not used inside the "run" script and not
> available to its child processes as they are not exported.
> Is it just so that if people add GIT_PERF_* variables to their
> config.mak before building they can then have those variables used by
> the run script?

Exactly. I put GIT_PERF_MAKE_OPTS in my config.mak, and they end up
respected for each run without me having to specify them manually.

> I also wonder if it would be better at step 3) to source the
> GIT-BUILD-OPTIONS file generated at step 2) instead of the original
> one, because they can be different as the options in
> $GIT_PERF_MAKE_OPTS will be baked into the new GIT-BUILD-OPTIONS file.
> (Of course if $GIT_PERF_MAKE_OPTS was added to config.mak before
> building, then they will be in the original one too. But
> $GIT_PERF_MAKE_OPTS should work without that.)

I think that would make some cases work (build options for the tested
build), but I fear that it would break others (perf variables that
probably should be coming from the "outer" layer). Remember that test
builds may not be current versions and may not be forwarding those
variables via GIT-BUILD-OPTIONS. I think it's important that the bundle
of t/perf scripts act as a single unit that is driven primarily by the
currently checked-out version (and it's up to those scripts to handle
inconsistencies in old versions; see the $MODERN_GIT stuff I added a few
months ago.

Right now I don't think it has been a big problem, because the build
config tends to be the same.  But if we introduce more "properties" that
the user can tweak for a certain test run, this distinction is probably
going to cause more bugs. I'd almost say that the perf scripts should be
a project outside of git.git entirely, to eliminate confusion.

-Peff

Re: [PATCH] sub-process: refactor handshake to common function

[Lars Schneider]

> On 24 Jul 2017, at 23:38, Jonathan Tan  wrote:
> 
> Refactor, into a common function, the version and capability negotiation
> done when invoking a long-running process as a clean or smudge filter.
> This will be useful for other Git code that needs to interact similarly
> with a long-running process.
> 
> Signed-off-by: Jonathan Tan 
> ---
> This will be useful for anyone implementing functionality like that in
> [1].
> 
> It is unfortunate that the code grew larger because it had to be more
> generic, but I think this is better than having (in the future) 2 or
> more separate handshake functions.
> 
> I also don't think that the protocol should be permissive - I think
> things would be simpler if all protocol errors were fatal errors. As it
> is, most parts are permissive, but packet_read_line() already dies
> anyway upon a malformed packet, so it may not be too drastic a change to
> change this. For reference, the original protocol comes from [2].
> 
> [1] 
> https://public-inbox.org/git/20170714132651.170708-2-benpe...@microsoft.com/
> [2] edcc858 ("convert: add filter..process option", 2016-10-17)

Thanks for this refactoring! That's great!

Please note that I've recently refactored the capabilities negotiation a bit:
https://github.com/git/git/commit/1514c8edd62d96006cd1de31e906ed5798dd4681

This change is still cooking in `next`. I am not sure how this should/could
be handled but maybe you can use my refactoring as your base?

Thanks,
Lars


> ---
> convert.c | 61 -
> pkt-line.c| 19 ---
> pkt-line.h|  2 --
> sub-process.c | 94 +++
> sub-process.h | 18 ++
> t/t0021-conversion.sh |  2 +-
> 6 files changed, 120 insertions(+), 76 deletions(-)
> 
> diff --git a/convert.c b/convert.c
> index deaf0ba7b..ec8ecc2ea 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -512,62 +512,15 @@ static struct hashmap subprocess_map;
> 
> static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)
> {
> - int err;
> + static int versions[] = {2, 0};
> + static struct subprocess_capability capabilities[] = {
> + {"clean", CAP_CLEAN}, {"smudge", CAP_SMUDGE}, {NULL, 0}
> + };
>   struct cmd2process *entry = (struct cmd2process *)subprocess;
> - struct string_list cap_list = STRING_LIST_INIT_NODUP;
> - char *cap_buf;
> - const char *cap_name;
> - struct child_process *process = >process;
> - const char *cmd = subprocess->cmd;
> -
> - sigchain_push(SIGPIPE, SIG_IGN);
> -
> - err = packet_writel(process->in, "git-filter-client", "version=2", 
> NULL);
> - if (err)
> - goto done;
> -
> - err = strcmp(packet_read_line(process->out, NULL), "git-filter-server");
> - if (err) {
> - error("external filter '%s' does not support filter protocol 
> version 2", cmd);
> - goto done;
> - }
> - err = strcmp(packet_read_line(process->out, NULL), "version=2");
> - if (err)
> - goto done;
> - err = packet_read_line(process->out, NULL) != NULL;
> - if (err)
> - goto done;
> -
> - err = packet_writel(process->in, "capability=clean", 
> "capability=smudge", NULL);
> -
> - for (;;) {
> - cap_buf = packet_read_line(process->out, NULL);
> - if (!cap_buf)
> - break;
> - string_list_split_in_place(_list, cap_buf, '=', 1);
> -
> - if (cap_list.nr != 2 || strcmp(cap_list.items[0].string, 
> "capability"))
> - continue;
> -
> - cap_name = cap_list.items[1].string;
> - if (!strcmp(cap_name, "clean")) {
> - entry->supported_capabilities |= CAP_CLEAN;
> - } else if (!strcmp(cap_name, "smudge")) {
> - entry->supported_capabilities |= CAP_SMUDGE;
> - } else {
> - warning(
> - "external filter '%s' requested unsupported 
> filter capability '%s'",
> - cmd, cap_name
> - );
> - }
> -
> - string_list_clear(_list, 0);
> - }
> -
> -done:
> - sigchain_pop(SIGPIPE);
> 
> - return err;
> + return subprocess_handshake(subprocess, "git-filter-", versions, NULL,
> + capabilities,
> + >supported_capabilities);
> }
> 
> static int apply_multi_file_filter(const char *path, const char *src, size_t 
> len,
> diff --git a/pkt-line.c b/pkt-line.c
> index 9d845ecc3..7db911957 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -171,25 +171,6 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
>   return status;
> }
> 
> -int packet_writel(int fd, const char *line, ...)
> -{
> - va_list args;
> - int err;
> - 

Re: [PATCH] git-gui (MinGW): make use of MSys2's msgfmt

[Lars Schneider]

> On 25 Jul 2017, at 10:35, Johannes Schindelin  
> wrote:
> 
> When Git for Windows was still based on MSys1, we had no gettext, ergo
> no msgfmt, either. Therefore, we introduced a small and simple Tcl
> script to perform the same task.
> 
> However, with MSys2, we no longer need that because we have a proper
> msgfmt executable. Plus, the po2msg.sh script somehow manages to hang
> when run in parallel in Git for Windows' SDK (symptom: the Continuous
> Testing tasks timing out).
> 
> Two reasons to use real msgfmt.exe instead.
> 
> Signed-off-by: Johannes Schindelin 
> ---
> 
>   This hopefully fixes the hangs with the Windows builds triggered
>   by Travis. It was a tough one to figure out originally, and it is

Awesome :-) Thanks for digging into this problem!

- Lars

Re: Reducing redundant build at Travis?

[Lars Schneider]

> On 21 Jul 2017, at 18:44, Junio C Hamano  wrote:
> 
> Lars Schneider  writes:
> 
>> To answer your question: I don't see an easy solution to the problem.
> 
> That's OK.  Thanks for digging.
> 
> I am wondering if the attached would be acceptable as a minimum
> impact patch to address this issue.  

Your patch would still run a number of expensive operations for the
same hashes (e.g. static analysis and documentation builds). I started 
to work on a patch that moves all TravisCI logic into scripts located 
in the `ci` folder. These scripts share a `lib-travisci.sh` for common 
functions such as `skip_branch_tip_with_tag ()` executed at the
beginning of every script.

Does this sound sensible to you? I am a bit busy with non Git related 
work right now but I try to post the patch for you to review ASAP.

- Lars

Re: [PATCH v1 0/4] Teach 'run' perf script to read config files

[Christian Couder]

On Fri, Jul 14, 2017 at 8:27 AM, Christian Couder
 wrote:
> On Thu, Jul 13, 2017 at 10:55 PM, Jeff King  wrote:
>> On Thu, Jul 13, 2017 at 08:57:01PM +0200, Christian Couder wrote:
>>
>>> >> We want to make it possible to store the parameters to the 'run'
>>> >> script in a config file. This will make it easier to store, reuse,
>>> >> share and compare parameters.
>>> >
>>> > Because perf-lib is built on test-lib, it already reads
>>> > GIT-BUILD-OPTIONS.
>>>
>>> Actually the 'run' script also sources GIT-BUILD-OPTIONS, so maybe
>>> this is not necessary.
>>
>> Ah, right. The one that comes via perf-lib gets the variables into the
>> test scripts themselves. But anything "run" would need itself would come
>> from the source it does itself. And that's where GIT_PERF_MAKE_OPTS has
>> an effect.
>>
>>> Also are the variables in GIT-BUILD-OPTIONS exported already?
>>
>> No, I don't think so. But because both "run" and the scripts themselves
>> source them, they're available more or less everywhere, except for
>> sub-processes inside the scripts.
>
> Ok, I see.

Actually after taking another look at that, it looks like the following happens:

1) the run script sources the original GIT-BUILD-OPTIONS file from
../.. relative to its location
2) a git version is built in "build/$rev" using GIT_PERF_MAKE_OPTS
which generates a new GIT-BUILD-OPTIONS file in "build/$rev/"
3) when the actual perf scripts are run they source the original
GIT-BUILD-OPTIONS file (through perf-lib.sh which sources test-lib.sh)

I wonder how useful 1) is, as the variables sourced from original
GIT-BUILD-OPTIONS are not used inside the "run" script and not
available to its child processes as they are not exported.
Is it just so that if people add GIT_PERF_* variables to their
config.mak before building they can then have those variables used by
the run script?

I also wonder if it would be better at step 3) to source the
GIT-BUILD-OPTIONS file generated at step 2) instead of the original
one, because they can be different as the options in
$GIT_PERF_MAKE_OPTS will be baked into the new GIT-BUILD-OPTIONS file.
(Of course if $GIT_PERF_MAKE_OPTS was added to config.mak before
building, then they will be in the original one too. But
$GIT_PERF_MAKE_OPTS should work without that.)

Re: git gc seems to break --symbolic-full-name

[Stas Sergeev]

26.07.2017 16:23, Jeff King пишет:

In git.git we do something like:

-- >8 --
other: version
cat $< >$@

.PHONY: FORCE
version: FORCE
@git rev-parse HEAD >$@+
@if cmp $@+ $@ >/dev/null 2>&1; then rm $@+; else mv $@+ $@; fi
-- >8 --

Yes, thats a nice recipe that I would be using
if not for the fact that I already switched to
"touch", which requires 1 fewer tmp file and
no comparison.
But I'll keep this in mind if something wrong
happens with my current solution, thanks!
I wonder if git can provide some helper script
for other projects to solve this in a same way.

Re: [PATCH/RFC] rebase: make resolve message clearer for inexperienced users

[Phillip Wood]

On 24/07/17 21:53, Junio C Hamano wrote:

Phillip Wood  writes:


git rebase --continue requiring one to git add first confuses/annoys me
too. I started a patch to autostage unstaged changes if they don't
contain conflict markers a couple of weeks ago, I'll clean it up and
post it later this week.


As long as "git rebase" will keep refusing to start in a working
tree with dirty files and/or index, this could be a good change.

But people _may_ be annoyed because they expect "--continue" to
remind them that some conflicts are not concluded with an explicit
"git add", and they would even feel that you made the command unsafe
if "--continue" just goes ahead by auto-adding their change that is
still work-in-progress.  Lack of conflict markers is not a sign that
a file is fully resolved (which they are used to signal by "git
add", and they do so per set of paths).


Thanks for your comments, I've tried to address them in the message with 
the patches I sent earlier today [1]. In summary autostaging is opt-in 
and the conflict marker check isn't perfect but it's better than nothing 
and covers an important case where the user has simply overlooked a 
conflict.



I also find it confusing that it asks me to edit the commit message for
picks, fixups and non-final squashes after conflicts. I can see that
perhaps one might want to amend the message to reflect any changes that
were made while resolving the conflicts but I've never had too. I'd
rather be able to pass --edit to rebase --continue if I needed to edit
the message in those cases. Looking through the code I think it would
require saving some extra state when rebase bails out on conflicts so
rebase --continue could tell if it should be asking the user to amend
the message.


This is disruptive if done without a careful transition plan and
you'll annoy existing users who expect to be able to edit by
default.  Especially since "rebase" keeps going and potentially
rebuild many commits on top, by the time they realize the mistake of
not passing "--edit", it is too late and they will hate you for
forcing them rebase many commits again.


I agree, I was imagining the new behaviour would be opt in via a config 
variable. Then if in the future there is a consensus to enable the new 
behaviour by default there would be a transition phase where users of 
the old behaviour would get a message telling that the behaviour is 
going to change in the future and what value to set the config variable 
to in order to keep the old behaviour if that's what they want.



If these suggestions above were given while "rebase -i" was
developed, it might have made the end-user experience a better one
than what it currently is, but transitioning after the current
behaviour has long been established makes it much harder.


Sadly I didn't even know git existed at the time rebase was first added.

Best Wishes

Phillip


[1] 
https://public-inbox.org/git/20170726102720.15274-2-phillip.w...@talktalk.net/T/#u

Re: git gc seems to break --symbolic-full-name

[Jeff King]

On Wed, Jul 26, 2017 at 11:40:46AM +0300, Stas Sergeev wrote:

> 26.07.2017 03:36, Jacob Keller пишет:
> > If your goal is to make it so users filling out bug reports have a
> > version, then using git descrsibe and making that be part of your
> > version (based off your tags, and commits) is how pretty much every
> > other project I've worked on does this.
> > 
> > I really don't think that's your goal here, given you're doing things
> > in make with timestamps and builds, so I guess I misunderstood your
> > answer?
> There are 2 different things:
> 1. put git describe output into some header file
> 2. make things to rebuild with every new commit
> 
> So I actually stuck at solving 2, because 1 is trivial.
> My original solution for 2 was to "depend" on
> refs/heads/*. This worked besides git gc, but had
> a lot of troubles with worktrees. And this time I
> switched to the "touch tmpfile" trick with the date
> taken from git log. This requires .LOW_RESOLUTION_TIME
> in makefile, so probably not the best solution again,
> but should hopefully be more future-proof than the
> previous one.

In git.git we do something like:

-- >8 --
other: version
cat $< >$@

.PHONY: FORCE
version: FORCE
@git rev-parse HEAD >$@+
@if cmp $@+ $@ >/dev/null 2>&1; then rm $@+; else mv $@+ $@; fi
-- >8 --

The "version" commands run always, but they only update the file if
there's a change. At least GNU make is smart enough to know that if
"version" was not updated, then "other" does not need to be re-built.
I don't know if all makes would so, though.

-Peff

[PATCH] remote: split and simplify messages

[Kaartic Sivaraam]

Splitting a single sentence across multiple lines could
degrade readability. Further, long messages are likely
to be ignored by users.

Split the sentences and simplify it to improve their readability.

Signed-off-by: Kaartic Sivaraam 
---
 remote.c   | 18 -
 t/t2020-checkout-detach.sh |  3 +-
 t/t7508-status.sh  | 96 +++---
 3 files changed, 58 insertions(+), 59 deletions(-)

diff --git a/remote.c b/remote.c
index 60d004392109f..e4e241d5e20f4 100644
--- a/remote.c
+++ b/remote.c
@@ -2093,10 +2093,10 @@ int format_tracking_info(struct branch *branch, struct 
strbuf *sb)
_("  (use \"git push\" to publish your local 
commits)\n"));
} else if (!ours) {
strbuf_addf(sb,
-   Q_("Your branch is behind '%s' by %d commit, "
-  "and can be fast-forwarded.\n",
-  "Your branch is behind '%s' by %d commits, "
-  "and can be fast-forwarded.\n",
+   Q_("Your branch is behind '%s' by %d commit.\n"
+  "It can be fast-forwarded.\n",
+  "Your branch is behind '%s' by %d commits.\n"
+  "It can be fast-forwarded.\n",
   theirs),
base, theirs);
if (advice_status_hints)
@@ -2104,12 +2104,10 @@ int format_tracking_info(struct branch *branch, struct 
strbuf *sb)
_("  (use \"git pull\" to update your local 
branch)\n"));
} else {
strbuf_addf(sb,
-   Q_("Your branch and '%s' have diverged,\n"
-  "and have %d and %d different commit each, "
-  "respectively.\n",
-  "Your branch and '%s' have diverged,\n"
-  "and have %d and %d different commits each, "
-  "respectively.\n",
+   Q_("Your branch and '%s' have diverged.\n"
+  "They have %d and %d different commit, 
respectively.\n",
+  "Your branch and '%s' have diverged.\n"
+  "They have %d and %d different commits, 
respectively.\n",
   ours + theirs),
base, ours, theirs);
if (advice_status_hints)
diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
index fbb4ee9bb42db..4e1c74c878560 100755
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -150,7 +150,8 @@ test_expect_success 'checkout does not warn leaving 
reachable commit' '
 '
 
 cat >expect <<'EOF'
-Your branch is behind 'master' by 1 commit, and can be fast-forwarded.
+Your branch is behind 'master' by 1 commit.
+It can be fast-forwarded.
   (use "git pull" to update your local branch)
 EOF
 test_expect_success 'tracking count is accurate after orphan check' '
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 43d19a9b22920..fd7f27ee01ab0 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -88,8 +88,8 @@ EOF
 test_expect_success 'status --column' '
cat >expect <<\EOF &&
 # On branch master
-# Your branch and '\''upstream'\'' have diverged,
-# and have 1 and 2 different commits each, respectively.
+# Your branch and '\''upstream'\'' have diverged.
+# They have 1 and 2 different commits, respectively.
 #   (use "git pull" to merge the remote branch into yours)
 #
 # Changes to be committed:
@@ -122,8 +122,8 @@ test_expect_success 'status --column 
status.displayCommentPrefix=false' '
 
 cat >expect <<\EOF
 # On branch master
-# Your branch and 'upstream' have diverged,
-# and have 1 and 2 different commits each, respectively.
+# Your branch and 'upstream' have diverged.
+# They have 1 and 2 different commits, respectively.
 #   (use "git pull" to merge the remote branch into yours)
 #
 # Changes to be committed:
@@ -199,8 +199,8 @@ test_expect_success 'commit ignores 
status.displayCommentPrefix=false in COMMIT_
 
 cat >expect <<\EOF
 On branch master
-Your branch and 'upstream' have diverged,
-and have 1 and 2 different commits each, respectively.
+Your branch and 'upstream' have diverged.
+They have 1 and 2 different commits, respectively.
 
 Changes to be committed:
new file:   dir2/added
@@ -272,8 +272,8 @@ test_expect_success 'status with gitignore' '
 
cat >expect <<\EOF &&
 On branch master
-Your branch and '\''upstream'\'' have diverged,
-and have 1 and 2 different commits each, respectively.
+Your branch and '\''upstream'\'' have diverged.
+They have 1 and 2 different commits, respectively.
   (use "git pull" to merge the remote branch into yours)
 
 Changes to be committed:
@@ -341,8 +341,8 @@ test_expect_success 'status with gitignore (nothing 
untracked)' '
 
 

Re: [PATCH] fsck: cleanup unused variable

[Jeff King]

On Tue, Jul 25, 2017 at 06:34:56PM -0700, Jonathan Tan wrote:

> Remove the unused variable "heads" from cmd_fsck().
> 
> This variable was made unused in commit c3271a0 ("fsck: do not fallback
> "git fsck " to "git fsck"", 2017-01-17).

Thanks, I should have caught this then.

The patch is obviously correct, though I'm curious why gcc's
-Wunused-but-set-variable didn't catch this.

-Peff

Re: [PATCH/RFC] setup: update error message to be more meaningful

[Kaartic Sivaraam]

Hello Jonathan Nieder,

Thanks for the reply!

Jonathan Nieder wrote:
> 
> > The error message shown when a flag is found when expecting a
> > filename wasn't clear as it didn't communicate what was wrong
> > using the 'suitable' words in *all* cases.
> > 
> > Correct case,
> > 
> > $ git rev-parse commit.c --flags
> > commit.c
> > --flags
> > fatal: bad flag '--flags' used after filename
> > 
> > Incorrect case,
> > 
> > $ git grep "test" -n
> > fatal: bad flag '-n' used after filename
> > 
> > Change the error message to be general and communicative.
> 
> Thanks for writing this.  These examples describe *what* the behavior
> is but don't describe *why* it is wrong or what is expected in its
> place.
> 
I've fixed that. The new commit message is found at the end of this
message.

> For an initial guess: in the example
> 
>   git grep test -n
> 
> it is confusing that it says "bad flag used after filename" because
> test isn't even a filename!  At first glance, I would imagine that any
> of the following behaviors would be nicer:
> 
>  1. Treat the command as "git grep -e test -n", or in other words
> "do what I mean" since it is clear enough, at least to humans.
> 
Sorry, I actually didn't that. Could you rephrase it a little.

>  2. Focus on "argument" instead of "filename" so that the message
> could still apply: something like
> 
>   fatal: option '-n' must come before non-option arguments
> 
I used "filename" as the function indeed check if the argument given to
it is a filename. How about,

fatal: expecting filename; found '-n'

???

In the context it looks as follows,

$ git grep "some random regex" -n
fatal: expected filename; found '-n' 

$ git rev-parse --flags
README.md
--flags
fatal: expected filename,
found '--flags'


> Probably because of the background I am missing described above, it's
> not clear to me that the new message is any better (or worse) than the
> existing one.  The old message with "after filename" has the virtue of
> explaining why an option is not expected there.
> 
That's surprising, I thought the phrase "in place of filename" was more
explanatory as it doesn't make assumptions about the previous arguments
and specifies what was expected.

> Thanks and hope that helps,
Yep


The modified commit message is found below. If it still seems to be
missing the *why*, let me know.

setup: update error message to be more meaningful

The error message shown when a flag is found when expecting a
filename wasn't clear as it didn't communicate what was wrong
using the 'suitable' words in *all* cases.

$ git ls-files
README.md
test-file

Correct case,

$ git rev-parse README.md --flags
README.md
--flags
fatal: bad flag '--flags' used after filename

Incorrect case,

$ git grep "some random regex" -n
fatal: bad flag '-n' used after filename

The above case is incorrect as "some random regex" isn't a filename
in this case.

Change the error message to be general and communicative. This results
in the following output,

$ git rev-parse README.md --flags
README.md
--flags
fatal: found flag '--flags' in place of a filename

$ git grep "some random regex" -n
fatal: found flag '-n' in place of a filename

-- 
Kaartic

[RFC PATCH 0/5] Add option to autostage changes when continuing a rebase

[Phillip Wood]

From: Phillip Wood 

These patches add an '--autostage' option (and corresponding config
variable) to 'rebase --continue' that will stage any unstaged changes
before continuing. This saves the user having to type 'git add' before
running 'git rebase --continue'.

Phillip Wood (5):
  rebase --continue: add --autostage to stage unstaged changes
  rebase -i: improve --continue --autostage
  Unify rebase amend message when HEAD has changed
  Add tests for rebase --continue --autostage
  Add rebase.continue.autostage config setting

 git-rebase--am.sh |   1 +
 git-rebase--interactive.sh| 111 --
 git-rebase--merge.sh  |   1 +
 git-rebase.sh |  76 ++---
 sequencer.c   |  22 +++--
 t/t3404-rebase-interactive.sh |   2 +-
 t/t3418-rebase-continue.sh|  50 ++-
 7 files changed, 222 insertions(+), 41 deletions(-)

-- 
2.13.3

[RFC PATCH 5/5] Add rebase.continue.autostage config setting

[Phillip Wood]

From: Phillip Wood 

This enables the user to always specify --autostage with --continue

The tests check that setting rebase.continue.autostage=true results in
'git rebase --continue' autostaging unstaged changes and that
'--no-autostage' overrides it.

Signed-off-by: Phillip Wood 
---
 git-rebase.sh  |  2 +-
 t/t3418-rebase-continue.sh | 15 +++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 
9ca387349b1cde440c4244de9125446fd35a7b67..632a19079ba1d88092f47121c7a0797af079aaf5
 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -70,7 +70,7 @@ merge_dir="$GIT_DIR"/rebase-merge
 apply_dir="$GIT_DIR"/rebase-apply
 verbose=
 diffstat=
-autostage=false
+autostage=$(git config --bool --get rebase.continue.autostage || echo false)
 test "$(git config --bool rebase.stat)" = true && diffstat=t
 autostash="$(git config --bool rebase.autostash || echo false)"
 fork_point=auto
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 
4e71e5d5c2f26866cd5d32bfea445f899398d6c9..7c2a30a3cd8ba7e33519bd68a3f89e90409169da
 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -67,6 +67,21 @@ test_autostage () {
git reset -- F2 &&
test_must_fail git rebase --continue --autostage
'
+
+   test_expect_success "rebase.continue.autostage rebase $action works" '
+   reset &&
+   test_must_fail git rebase $action --onto master master topic &&
+   echo "Resolved" >F2 &&
+   git -c rebase.continue.autostage=true rebase --continue
+'
+
+   test_expect_success "rebase $action --continue --no-autostage works" '
+   reset &&
+   test_must_fail git rebase $action --onto master master topic &&
+   echo "Resolved" >F2 &&
+   test_must_fail git -c rebase.continue.autostage=true rebase 
--continue --no-autostage
+'
+
 }
 
 test_autostage
-- 
2.13.3

[RFC PATCH 4/5] Add tests for rebase --continue --autostage

[Phillip Wood]

From: Phillip Wood 

Make sure that --autostage stages any changes and fails if there are
merge markers in the file to be staged.

Signed-off-by: Phillip Wood 
---
 t/t3418-rebase-continue.sh | 35 ++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 
4428b9086e8bcb383df801834d0de323f316f4fa..4e71e5d5c2f26866cd5d32bfea445f899398d6c9
 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -14,7 +14,6 @@ test_expect_success 'setup' '
 
git checkout -b topic HEAD^ &&
test_commit "commit-new-file-F2-on-topic-branch" F2 22 &&
-
git checkout master
 '
 
@@ -40,6 +39,40 @@ test_expect_success 'non-interactive rebase --continue works 
with touched file'
git rebase --continue
 '
 
+reset () {
+   rm -fr .git/rebase-* &&
+   git reset --hard commit-new-file-F2-on-topic-branch &&
+   git checkout master
+}
+
+test_autostage () {
+   action=$1
+
+   test_expect_success "rebase $action --continue --autostage stages 
changes" '
+   reset &&
+   test_must_fail git rebase $action --onto master master topic &&
+   echo "Resolved" >F2 &&
+   git rebase --continue --autostage
+'
+
+   test_expect_success "rebase $action --continue --autostage fails when 
there are merge markers (1)" '
+   reset &&
+   test_must_fail git rebase $action --onto master master topic &&
+   test_must_fail git rebase --continue --autostage
+'
+
+   test_expect_success "rebase $action --continue --autostage  fails when 
there are merge markers (2)" '
+   reset &&
+   test_must_fail git rebase $action --onto master master topic &&
+   git reset -- F2 &&
+   test_must_fail git rebase --continue --autostage
+   '
+}
+
+test_autostage
+test_autostage -i
+test_autostage -m
+
 test_expect_success 'non-interactive rebase --continue with rerere enabled' '
test_config rerere.enabled true &&
test_when_finished "test_might_fail git rebase --abort" &&
-- 
2.13.3

[RFC PATCH 2/5] rebase -i: improve --continue --autostage

[Phillip Wood]

From: Phillip Wood 

If HEAD has changed since the rebase stopped or rebase stopped due to
a failed exec then 'git rebase --continue --autostage' will autostage
changes that cannot be commited. Fix this by reordering some of the
checks so that 'git rebase --continue --autostage' never stages
changes unless they can be committed. Also reword some error
messages to account of --autostage and try and make them clearer.

Signed-off-by: Phillip Wood 
---

This could do with some tests the check the correct error message is
given in each failure case. However I'm expecting to change the error
messages based on the feedback I get to this patch so I'll add the
tests once the messages are finalized. I've tried to give a bit more
explanation in the error messages as I found some of the previous
messages didn't explain why rebase couldn't continue. Splitting the
advice out means it is consistent between different code paths and
should make it easier to optionally disable it in future if anyone
wanted to add that. As for the formatting of the messages I wonder if
they would be better without so many empty lines (I find the current
messages a bit intimating as they seem so long). e.g.

If you wish to squash the unstaged changes into the last commit, run:
git add -u
git commit --amend \$gpg_sign_opt_quoted
If they are meant to go into a new commit, run:
git add -u
git commit \$gpg_sign_opt_quoted
In both cases, once you're done, continue with:
git rebase --continue

Instead of

If you wish to squash the unstaged changes into the last commit, run:

  git add -u
  git commit --amend \$gpg_sign_opt_quoted

If they are meant to go into a new commit, run:

  git add -u
  git commit \$gpg_sign_opt_quoted

In both cases, once you're done, continue with:

  git rebase --continue


 git-rebase--interactive.sh| 104 --
 t/t3404-rebase-interactive.sh |   2 +-
 2 files changed, 81 insertions(+), 25 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 
4c037a3f7a9e01406c4205bf8786a3da5060381f..8140c88839b4f3a86f53f2ba4433ecc7f58b
 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1056,6 +1056,42 @@ The possible behaviours are: ignore, warn, error.")"
fi
 }
 
+unstaged_advice () {
+   gpg_sign_opt_quoted=${gpg_sign_opt:+$(git rev-parse --sq-quote 
"$gpg_sign_opt")}
+   eval_gettext "\
+If you wish to squash the unstaged changes into the last commit, run:
+
+  git add -u
+  git commit --amend \$gpg_sign_opt_quoted
+
+If they are meant to go into a new commit, run:
+
+  git add -u
+  git commit \$gpg_sign_opt_quoted
+
+In both cases, once you're done, continue with:
+
+  git rebase --continue
+"
+}
+
+staged_advice () {
+   gpg_sign_opt_quoted=${gpg_sign_opt:+$(git rev-parse --sq-quote 
"$gpg_sign_opt")}
+   eval_gettext "\
+If you wish to squash the staged changes into the last commit, run:
+
+  git commit --amend \$gpg_sign_opt_quoted
+
+If they are meant to go into a new commit, run:
+
+  git commit \$gpg_sign_opt_quoted
+
+In both cases, once you're done, continue with:
+
+  git rebase --continue
+"
+}
+
 # The whole contents of this file is run by dot-sourcing it from
 # inside a shell function.  It used to be that "return"s we see
 # below were not inside any function, and expected to return
@@ -1067,9 +1103,50 @@ The possible behaviours are: ignore, warn, error.")"
 # here, and immediately call it after defining it.
 git_rebase__interactive () {
 
+amend_head='' amend_ok=''
 case "$action" in
 continue)
-   check_unstaged
+   test -f "$amend" &&
+   amend_head=$(cat "$amend") &&
+   test $amend_head = $(git rev-parse HEAD) &&
+   amend_ok=1
+   git update-index --refresh --ignore-submodules >/dev/null
+   git diff-files --quiet --ignore-submodules
+   unstaged=$?
+   if ! test -f "$author_script"
+   then
+   if test $unstaged = 1
+   then
+   die "$(gettext "Not expecting unstaged changes.")
+$(unstaged_advice)"
+   elif ! git diff-index --cached --quiet --ignore-submodules HEAD 
--
+   then
+   die "$(gettext "Not expecting staged changes.")
+$(staged_advice)"
+   fi
+   fi
+   if test $unstaged = 1 && test $autostage = true
+   then
+   if test -n "$amend_head" && test -z "$amend_ok"
+   then
+   die "$(gettext "\
+Unable to commit changes as HEAD has changed since git rebase stopped.")
+$(unstaged_advice)"
+   else
+   check_autostage
+   fi
+   elif test $unstaged = 1
+   then
+   if test -n "$amend_head" && test -z "$amend_ok"
+   then
+   die "$(gettext "\
+Unable to continue rebasing as there are 

[RFC PATCH 1/5] rebase --continue: add --autostage to stage unstaged changes

[Phillip Wood]

From: Phillip Wood 

After resolving conflicts during a rebase it is a pain to have to run
'git add' before 'git rebase --continue'. Passing --autostage to 'git
rebase --continue' will stage them automatically so long as 'git diff
--check' says they are ok. This is a safety measure to avoid
accidentally staging files containing unresolved conflicts.

Continuing an interactive rebase after a failed exec or if HEAD has
changed since rebase stopped with --autostage will stage changes that
wont be committed as rebase --continue will bail out. This will be
fixed in the next commit.

Signed-off-by: Phillip Wood 
---

Using diff check is not ideal as it will error out on whitespace
changes (which should be check in a commit hook if the user is worried
about them) as well as merge markers. Looking through diff.c it should
be possible to add a --check=merge-markers optional argument as the
two checks are enabled independently in the code. As Junio pointed out
in a previous message [1] the absence of conflict markers does not
indicate that a file is fully resolved but checking for them does at
least avoid the case of the user blindly continuing when they have
forgotten to look at a conflicted file.

The autostaging behaviour is opt-in so users who like the additional
safety of having to do git add before git rebase --continue can
continue using their current workflow.

I wonder if check_unstaged() should give different error messages
depending on the presence of unmerged paths rather than saying
'unstaged changes' all the time. Also it should probably have some
message after the check for merge markers fails rather than just the
raw output from diff --check.

[1] https://public-inbox.org/git/xmqqmv7td0a5@gitster.mtv.corp.google.com/


 git-rebase--am.sh  |  1 +
 git-rebase--interactive.sh |  1 +
 git-rebase--merge.sh   |  1 +
 git-rebase.sh  | 76 +-
 4 files changed, 71 insertions(+), 8 deletions(-)

diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index 
375239341fbfe885e51a25e9e0dc2d4fee791345..30faa8c24cce2149a883c0055e3f6e93dabd2dd0
 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -17,6 +17,7 @@ git_rebase__am () {
 
 case "$action" in
 continue)
+   check_unstaged
git am --resolved --resolvemsg="$resolvemsg" \
${gpg_sign_opt:+"$gpg_sign_opt"} &&
move_to_original_branch
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 
90b1fbe9cf6e8dfb2f4331916809fa40bf9050d2..4c037a3f7a9e01406c4205bf8786a3da5060381f
 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1069,6 +1069,7 @@ git_rebase__interactive () {
 
 case "$action" in
 continue)
+   check_unstaged
if test ! -d "$rewritten"
then
exec git rebase--helper ${force_rebase:+--no-ff} --continue
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index 
06a4723d4db3db74ea17ace60d824e83cdee25e9..81723fc485882750c3ed7214b880d49cf55c6d68
 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -16,6 +16,7 @@ read_state () {
 continue_merge () {
test -d "$state_dir" || die "$state_dir directory does not exist"
 
+   check_unstaged
unmerged=$(git ls-files -u)
if test -n "$unmerged"
then
diff --git a/git-rebase.sh b/git-rebase.sh
index 
2cf73b88e8e83ca34b9eb319dbc2b0a220139b0f..9ca387349b1cde440c4244de9125446fd35a7b67
 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -9,7 +9,7 @@ OPTIONS_STUCKLONG=t
 OPTIONS_SPEC="\
 git rebase [-i] [options] [--exec ] [--onto ] [] 
[]
 git rebase [-i] [options] [--exec ] [--onto ] --root []
-git-rebase --continue | --abort | --skip | --edit-todo
+git-rebase --continue [--autostage] | --abort | --skip | --edit-todo
 --
  Available options are
 v,verbose! display a diffstat of what changed upstream
@@ -32,6 +32,7 @@ verify allow pre-rebase hook to run
 rerere-autoupdate  allow rerere to update index with resolved conflicts
 root!  rebase all reachable commits up to the root(s)
 autosquash move commits that begin with squash!/fixup! under -i
+a,autostageadd unstaged changes to the index when continuing
 committer-date-is-author-date! passed to 'git am'
 ignore-date!   passed to 'git am'
 signoffpassed to 'git am'
@@ -69,6 +70,7 @@ merge_dir="$GIT_DIR"/rebase-merge
 apply_dir="$GIT_DIR"/rebase-apply
 verbose=
 diffstat=
+autostage=false
 test "$(git config --bool rebase.stat)" = true && diffstat=t
 autostash="$(git config --bool rebase.autostash || echo false)"
 fork_point=auto
@@ -213,6 +215,67 @@ run_pre_rebase_hook () {
fi
 }
 
+check_autostage () {
+   # If the user has already staged files that contain whitespace
+   # errors or merge markers then we want ignore them so rebase
+   # --continue behaves consistency with and without --autostage
+   

[RFC PATCH 3/5] Unify rebase amend message when HEAD has changed

[Phillip Wood]

From: Phillip Wood 

If rebase --interactive is unable to commit staged changes because
HEAD has changed since rebase stopped the user gets different messages
depending on whether they specified --autostage or not. Update the
messages in the other code paths to match the --autostage one.

Signed-off-by: Phillip Wood 
---

The change from error() to fprintf() is to keep the messages consistent,
maybe the messages in the shell script should be prefixed with
'error:' instead.

 git-rebase--interactive.sh | 10 ++
 sequencer.c| 22 +-
 2 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 
8140c88839b4f3a86f53f2ba4433ecc7f58b..e1845e940b8de05b10b011d8167917a60a7c00b9
 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1164,10 +1164,12 @@ $(unstaged_advice)"
die "$(gettext "Error trying to find the author 
identity to amend commit")"
if test -n "$amend_head"
then
-   test -n "$amend_ok" ||
-   die "$(gettext "\
-You have uncommitted changes in your working tree. Please commit them
-first and then run 'git rebase --continue' again.")"
+   test -n "$amend_ok" || {
+   gpg_sign_opt_quoted=${gpg_sign_opt:+$(git 
rev-parse --sq-quote "$gpg_sign_opt")}
+   die "$(gettext "\
+Unable to commit changes as HEAD has changed since git rebase stopped.")
+$(staged_advice)"
+   }
do_with_author git commit --amend --no-verify -F "$msg" 
-e \
${gpg_sign_opt:+"$gpg_sign_opt"} ||
die "$(gettext "Could not commit staged 
changes.")"
diff --git a/sequencer.c b/sequencer.c
index 
3010faf86398697469e903318a35421d911acb23..2722d36781e5c47ee81eb3359aa6178042430e68
 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2214,12 +2214,24 @@ static int commit_staged_changes(struct replay_opts 
*opts)
if (get_sha1_hex(rev.buf, to_amend))
return error(_("invalid contents: '%s'"),
rebase_path_amend());
-   if (hashcmp(head, to_amend))
-   return error(_("\nYou have uncommitted changes in your "
-  "working tree. Please, commit them\n"
-  "first and then run 'git rebase "
-  "--continue' again."));
+   if (hashcmp(head, to_amend)) {
+   const char *gpg_opt = gpg_sign_opt_quoted(opts);
 
+   fprintf(stderr, _(
+"Unable to commit changes as HEAD has changed since git rebase stopped.\n"
+"If you wish to squash the changes into the last commit, run:\n"
+"\n"
+"  git commit --amend %s\n"
+"\n"
+"If they are meant to go into a new commit, run:\n"
+"\n"
+"  git commit %s\n"
+"\n"
+"In both cases, once you're done, continue with:\n"
+"\n"
+"  git rebase --continue\n"), gpg_opt, gpg_opt);
+   return -1;
+   }
strbuf_release();
flags |= AMEND_MSG;
}
-- 
2.13.3

Loan Offer@2%

[Daniel Jones]

I am a private Lender Daniel Jones located in USA, Are You In Need Of
A Private Or Business Loans from $5,000.00 to $10.000.000.00 At 2%
Rate For Various Purposes? If Yes; Contact us Via Email:
3231...@gmail.com
Names...
Amount($)...
Country 
Tel...
Duration...

Urgent reply needed,

[Amuji Gab,]

Dear Partner,

Please consider this mail serious despite the fact that you did not
expect it. Hope you are doing well. I am Mr.Alimuji Gabratai, the
Manager of head opérations department of our bank in Burkina Faso. I
discovered a risk-free deal of US$9.9 million from my department which
was left unclaimed as a result of non existing body.Provided you will
put trust forward, let us share the deal if you are interested.

Urgent reply is needed for more details.Reply to: amuji...@gmail.com,

Regards,

Mr.Alimuji Gabratai,

Re: git gc seems to break --symbolic-full-name

[Stas Sergeev]

26.07.2017 03:36, Jacob Keller пишет:

If your goal is to make it so users filling out bug reports have a
version, then using git descrsibe and making that be part of your
version (based off your tags, and commits) is how pretty much every
other project I've worked on does this.

I really don't think that's your goal here, given you're doing things
in make with timestamps and builds, so I guess I misunderstood your
answer?

There are 2 different things:
1. put git describe output into some header file
2. make things to rebuild with every new commit

So I actually stuck at solving 2, because 1 is trivial.
My original solution for 2 was to "depend" on
refs/heads/*. This worked besides git gc, but had
a lot of troubles with worktrees. And this time I
switched to the "touch tmpfile" trick with the date
taken from git log. This requires .LOW_RESOLUTION_TIME
in makefile, so probably not the best solution again,
but should hopefully be more future-proof than the
previous one.

Re: [RFC] Git rerere and non-conflicting changes during conflict resolution

[Junio C Hamano]

Jeff King  writes:

> Hrm. That doesn't quite work, though. Because if your  are the
> merge, then merging a topic to next will get an "A" that is a merge
> commit from next. But that commit will never end up in master. What's
> causing the conflict is really some "A" that is in the history between
> the merge base and "A" (but we don't know which).

There may be a misunderstanding.  When I said the key  is a
pair of branch names, I didn't mean 'A' to be the name of an
integration branch (e.g. 'pu') and 'B' to be the name of a topic.
Rather, both 'A' and 'B' are the names of topic branches.  

IOW, instead of having refs/merge-fix/sd/branch-copy that says "I
know when I merge sd/branch-copy to pu or jch, there is a semantic
conflict with some unnamed topic that is likely to be already in
there", i.e. keying with only a single topic name, the ideal I
presented would say 'sd/branch-copy and mh/packed-ref-store topics
are both by themselves OK, but when merged together, the end result
of textual merge needs to be further fixed up by cherry-picking this
change', by keying a change with a pair of topic names,
sd/branch-copy (which introduces a new method in the ref backend
vtable) and mh/packed-ref-store (which adds a new ref backend).  The
latter does not know the need for the new method, and the former
does not know the need to implement its new method in a new backend,
so a merge needs a trivial implementation of the new method added to
the new backend, which is what refs/merge-fix/sd/branch-copy does.

And better yet, instead of A=sd/branch-copy B=mh/packed-ref-store,
we could point at the exact commit on each of these branches that
introduce the semantic conflict.  I would probably pick these two

  A=52d59cc6 ("branch: add a --copy (-c) option to go with --move (-m)", 
2017-06-18)
  B=67be7c5a ("packed-backend: new module for handling packed references", 
2017-06-23)

so when we are on commit X that has A but not B, and are trying to
merge branch Y that has B but not A, we want the merge-fix to kick
in.  Walking "rev-list --left-right X...Y" and noticing A and B in
the output would be a way to notice it.


[footnote]

*1* https://github.com/gitster/git/ should mirror these refs in the
refs/merge-fix/ hierarchymentioned in the body of this article.

[PATCH v4] merge: add a --signoff flag.

[Łukasz Gryglicki]

Some projects require every commit, even merges, to be signed off [*1*].
Because "git merge" does not have a "--signoff"
option like "git commit" does, the user needs to add one
manually when the command presents an editor to describe the
merge, or later use "git commit --amend --signoff".

Help developers of these projects by teaching "--signoff" option to "git merge".

*1*
https://public-inbox.org/git/CAHv71zK5SqbwrBFX=a8-dy9h3kt4feymgv__p2gzznr0wua...@mail.gmail.com/T/#u

Requested-by: Dan Kohn 
Signed-off-by: Łukasz Gryglicki 
---
 Documentation/git-merge.txt |  8 +
 builtin/merge.c |  4 +++
 t/t7614-merge-signoff.sh| 88 +
 3 files changed, 100 insertions(+)
 create mode 100755 t/t7614-merge-signoff.sh

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 04fdd8cf086db..630cb4b7f90d7 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -64,6 +64,14 @@ OPTIONS
 ---
 include::merge-options.txt[]
 
+--signoff::
+   Add Signed-off-by line by the committer at the end of the commit
+   log message.  The meaning of a signoff depends on the project,
+   but it typically certifies that committer has
+   the rights to merge work under the same license and
+   agrees to a Developer Certificate of Origin
+   (see http://developercertificate.org/ for more information).
+
 -S[]::
 --gpg-sign[=]::
GPG-sign the resulting merge commit. The `keyid` argument is
diff --git a/builtin/merge.c b/builtin/merge.c
index 900bafdb45d0b..78c36e9bf353b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -70,6 +70,7 @@ static int continue_current_merge;
 static int allow_unrelated_histories;
 static int show_progress = -1;
 static int default_to_upstream = 1;
+static int signoff;
 static const char *sign_commit;
 
 static struct strategy all_strategy[] = {
@@ -233,6 +234,7 @@ static struct option builtin_merge_options[] = {
{ OPTION_STRING, 'S', "gpg-sign", _commit, N_("key-id"),
  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
OPT_BOOL(0, "overwrite-ignore", _ignore, N_("update ignored 
files (default)")),
+   OPT_BOOL(0, "signoff", , N_("add Signed-off-by:")),
OPT_END()
 };
 
@@ -763,6 +765,8 @@ static void prepare_to_commit(struct commit_list 
*remoteheads)
strbuf_addch(, '\n');
if (0 < option_edit)
strbuf_commented_addf(, _(merge_editor_comment), 
comment_line_char);
+   if (signoff)
+   append_signoff(, ignore_non_trailer(msg.buf, msg.len), 0);
write_file_buf(git_path_merge_msg(), msg.buf, msg.len);
if (run_commit_hook(0 < option_edit, get_index_file(), 
"prepare-commit-msg",
git_path_merge_msg(), "merge", NULL))
diff --git a/t/t7614-merge-signoff.sh b/t/t7614-merge-signoff.sh
new file mode 100755
index 0..823d840d423df
--- /dev/null
+++ b/t/t7614-merge-signoff.sh
@@ -0,0 +1,88 @@
+#!/bin/sh
+
+test_description='git merge --signoff
+
+This test runs git merge --signoff and makes sure that it works.
+'
+
+. ./test-lib.sh
+
+# Setup test files
+test_setup () {
+  # Expected commit message after merge --signoff
+  printf "Merge branch 'master' into other-branch\n\n" >expected-signed &&
+  printf "Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/")\n" 
>>expected-signed &&
+
+  # Expected commit message after merge without --signoff (or with 
--no-signoff)
+  echo "Merge branch 'master' into other-branch" >expected-unsigned &&
+
+  # Initial commit and feature branch to merge master into it.
+  git commit --allow-empty -m "Initial empty commit" &&
+  git checkout -b other-branch &&
+  test_commit other-branch file1 1
+}
+
+# Create fake editor that just copies file
+create_fake_editor () {
+  echo 'cp "$1" "$1.saved"' | write_script save-editor
+}
+
+test_expect_success 'setup' '
+  test_setup && create_fake_editor
+'
+
+test_expect_success 'git merge --signoff adds a sign-off line' '
+  git checkout master &&
+  test_commit master-branch-2 file2 2 &&
+  git checkout other-branch &&
+  git merge master --signoff --no-edit &&
+  git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+  test_cmp expected-signed actual
+'
+
+test_expect_success 'git merge does not add a sign-off line' '
+  git checkout master &&
+  test_commit master-branch-3 file3 3 &&
+  git checkout other-branch &&
+  git merge master --no-edit &&
+  git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+  test_cmp expected-unsigned actual
+'
+
+test_expect_success 'git merge --no-signoff flag cancels --signoff flag' '
+  git checkout master &&
+  test_commit master-branch-4 file4 4 &&
+  git checkout other-branch &&
+  git merge master --no-edit --signoff --no-signoff &&
+  git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+  test_cmp expected-unsigned actual
+'
+
+test_expect_success 'git 

Re: [PATCH v3] merge: add a --signoff flag.

[Junio C Hamano]

lukaszgryglicki  writes:

>>> +--signoff::
>>> +   Add Signed-off-by line by the committer at the end of the commit
>>> +   log message.  The meaning of a signoff depends on the project,
>>> +   but it typically certifies that committer has
>>> +   the rights to submit this work under the same license and
>>> +   agrees to a Developer Certificate of Origin
>>> +   (see http://developercertificate.org/ for more information).
>> 
>> This is taken verbatim from Documentation/git-commit.txt and "this
>> work" in that context is entirely sensible, but it is not quite
>> clear what it means in the context of "git merge”.
>
> Changed slightly, but I’m not a native English speaker, and
> don’t really see what is wrong there.

Oh, the language is not the issue.  

I was trying to remind you that a "merge" may be an automated and
mechanical one, in which there is no original work that requires
signing it off in the first place, which was the point Ævar raised
when Dan's original post came up.

Re: [RFC] Git rerere and non-conflicting changes during conflict resolution

[Junio C Hamano]

Jeff King  writes:

> From the user's perspective, calling X "rerere" would probably be OK[1].
> But from an implementation perspective (and to keep the existing
> plumbing available and unchanged), it probably makes sense to call it
> something else, and have it run both rerere and a new plumbing command
> to do the merge-fix work (or call it nothing, and assume that users will
> either touch the plumbing directly or will use "git merge" to trigger
> both).
> ...
> I think it should be its own plumbing tool that merge calls alongside
> rerere. ;)

As long as we use the database keyed with  and take the merge
base into account, "git am" and "git cherry-pick" would not be able
to use the merge-fix machinery, so in that sense, calling X "rerere"
would not be OK, but I agree with your general sentiment about the
UI visible to the end users.  Just like "rerere" started with a
small step to avoid automating things too much and then later became
almost invisible for normal cases because we managed to automate it
reasonably well and integrate it into mergy operations, we may be
able to do the same for merge-fix machinery.  My "this belongs to
'merge'" is primarily coming from it---it might be reusable in other
mergy operations with some fundamental changes, but I envision that
the primarly and only user of that X would initially be 'merge'.

> Not having thought too hard about it yet, this containing relationship
> seems like the right direction. I guess you'd do the lookup by computing
> the merge-base M of  (which we already know anyway), walking M..X
> and looking for any entries which mention those commits (in either A or
> B slots of the entry), and then similarly narrowing it according to
> M..Y.

Yes, every time I look at the Reintegrate script, I try to think of
an efficient implementation but do not find anything better than the
left-right walk over X...Y range, so that is my conclusion after
having thought about it very hard as well ;-)

> What if instead of commit hashes we used patch ids?

Now you may be onto something.  While we aim at the ultimately automated
ideal that would catch the maximal cases, for the earlier 'xyzzy
turns into frotz' example, the minimal cue to identify one side of
the pair that keys the "change this new instance of xyzzy into
frotz, too" merge-fix is a hunk like this:

-const char *xyzzy;
+const char *frotz;

It does not matter what other changes also appear in the same
commit, and my original "branch name" is way too broad, and my
previous "ideal" which is "a single problematic commit" is still
broader than necessary.  Well, patch-id identifies the entire change
contained in a single commit, so it is still too broad, but if we
can narrow it down to a single hunk like that, perhaps we can use it
as one side of the key.

And the other side of the key is naturally a hunk like this:

+   printf("%s\n", xyzzy);

i.e. a new use of xyzzy appears where it didn't exist before.  And
when we merge two branches, one of which has one half of the key
(i.e. "const char *xyzzy;" turned into "const char *frotz"), and the
other has the other half of the key (i.e. "printf xyzzy" is added),
then we'd apply a patch that tells us to do this:

-   printf("%s\n", xyzzy);
+   printf("%s\n", frotz);

i.e. that patch would be the value in the database keyed by the pair
of two previous hunks.

> I think it's asking a lot for users to handle the textual conflicts and
> semantic ones separately. It would be nice if we could tell them apart
> automatically (and I think we can based on what isn't part of the
> conflict resolution).

After thinking about this a bit more, I realized that it is possible
to mechanically sift a human generated resolution that has both
textual conflict resolution (which will be handled by "rerere") and
semantic one (which needs the merge-fix machinery) into two, without
requiring the user to make two separate commits.

The key trick is that "rerere" is capable of recording and replaying
semantic conflict resolution made to a file that happens to have
textual conflict just fine.  Because rerere database records the
preimage (i.e. with conflict markers) and postimage (i.e. the final
resolution for the file) as an entire file contents, it can do 3-way
merge for parts of the same file that is away from any conflicted
region.

If you tell contrib/rerere-train.sh to learn from "pu^{/^Merge
branch 'bp/fsmonitor' into pu}", you'll see what I mean.

  $ git show "pu^{/^Merge branch 'bp/fsmonitor' into pu}" compat/bswap.h

shows the result of such resolution.  Early part of the combined
diff shown by the above command comes from textual conflict
resolution, but there is a new implementation of inline version of
get_be64 that has become necessary but did not exist in either of
the branches getting merged, which is shown as an evil merge.

So the way to automatically sift an existing merge into textual
conflict resolution (i.e. automatable with