Re: "git add -p" versus "git add -i", followed by "p"

[Kevin Daudt]

On Sun, Dec 02, 2018 at 11:30:19AM -0500, Robert P. J. Day wrote:
> 
>   testing adding by patch for the very first time (i've just never
> needed this), and reading the "progit" book and reading the man page,
> and the impression i'm getting is that running "git add -p" (going
> straight to patch mode) is supposed to be equivalent to running "git
> add -i", then typing "p" to switch to patch mode.
> 
>   that is most emphatically not what i'm seeing. if i run "git add
> -p", then i get to what i expect -- the patch subsystem:
> 
>   $ git add -p
>   diff --git a/README.asc b/README.asc
>   index fa40bad..840e85b 100644
>   --- a/README.asc
>   +++ b/README.asc
>   @@ -1,3 +1,9 @@
>   +change 1
>   +
>   +
>   +
>   +
>   +
>= Pro Git, Second Edition
> 
>Welcome to the second edition of the Pro Git book.
>   Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?
> 
> but if i start with "git add -i", there seems to be no way to get to
> patch mode -- certainly "p" doesn't do it. am i stupidly missing
> something trivial? is the explanation misleading or inncomplete?
> 
> rday
> 
> -- 
> 

After selecting 'p', what do you get?

You should see a list of modified files. You can select the files you
want to stage by the listed numbers. After you selected those files, you
press enter, and then you will get the options you'll also see with git
add -p.

Kevin

Re: Understanding Index Header

[Kevin Daudt]

On Sat, Oct 06, 2018 at 05:13:45PM -0400, Farhan Khan wrote:
> Hi all,
> 
> I am writing a program that parses out the .git/index file. I am
> reading the git index header documentation, but I seem to be getting
> jibberish data.
> https://github.com/git/git/blob/master/Documentation/technical/index-format.txt
> 
> The first 12 bytes are the signature, version and entries. Great so far.
> 
> Afterwards, I try to read the extension signature, which the
> documentation says: "4-byte extension signature. If the first byte is
> 'A'..'Z' the
> extension is optional and can be ignored.". I am getting jibberish. Is
> this expected?
> 
> Then, the extension size comes back as 3556 (after ntohl()). That
> seems extremely high. My structure is as follows:
> 
> struct _indexfile_hdr {
> unsigned char   sig[4]; /* Always "DIRC" */
> uint32_tversion;/* Version Number */
> uint32_tentries;/* Number of extensions */
> unsigned char   extsig[4];  /* Extension signature */
> uint32_textsize;/* Size of the extension */
> uint8_t sha[8]; /* SHA1 of index before checksum */
> } __packed;
> 
> Am I doing something wrong? Is there some offset or padding that I missed?
> 
> Thanks,
> --
> Farhan Khan

Hello Farhan

Between the index header and the extensions there are index entries. So
you have 4 sections:

1. Header (fixed size)
2. Index entries (dynamic size, amount stored in the index)
3. Extensions (dynamic size)
4. hash

So you cannot capture the index with just a single struct. Also what you
call the 'entries' field is not the number of extensions, but the number
of index entries.

I'm not sure why you are seeing so many index entries. I've just tested
it myself on the git repository and I got 3419 entries when reading in
network (big-endian) order.

Hope this helps a bit.

Kevin

Re: Mailsplit

[Kevin Daudt]

On Wed, Sep 05, 2018 at 09:17:29PM -0700, Stephen & Linda Smith wrote:
> I thought I would use "git mailsplit" to split a mbox file (which downloaded 
> from public inbox) so that I could attemp to resurrect a patch series for 
> from 
> a year ago.  
> 
> The motivation is that I downloaded the series [1] and applied to a tag from 
> about the time period that the patch was sent out [2].  
> 
> The "git am -3 patch.mbox  quit 2/3 of the way though.   I resolved the fix. 
> and ran "git am --continue" which didn't apply the rest of the patches in the 
> mbox.
> 
> So two questions:
> 1)  why would git version 2.18.0 not appear to continue applying the patches. 
>   
> 
> 2) where do I find the command "git mailsplit".   The onlything in my 
> installed tree is:
> 
>   $ find  /usr/local/ -name '*mailsplit*'
>   /usr/local/share/doc/git-doc/git-mailsplit.txt
>   /usr/local/share/doc/git-doc/git-mailsplit.html
>   /usr/local/share/man/man1/git-mailsplit.1
>   /usr/local/libexec/git-core/git-mailsplit

This is the mailsplit command, and should be executed when running `git
mailsplit`. What does git --exec-dir return?

Kevin

Re: Adding nested repository with slash adds files instead of gitlink

[Kevin Daudt]

On Mon, Jun 18, 2018 at 01:19:19PM +0200, Heiko Voigt wrote:

Now with cc to the mailing list.

> Hi,
> 
> I just discovered that when you have a slash at the end of a nested
> repository, the files contained in the repository get added instead of
> the gitlink.
> 
> I found this when I was adding a submodule and wanted to commit a small
> change before that. You get the slash by using tab autocompletion.
> 
> Here is a recipe to reproduce:
> 
> mkdir test
> cd test; git init
> touch a; git add a; git commit -m a
> mkdir ../test.git; (cd ../test.git; git init --bare)
> git remote add origin ../test.git
> git push origin master
> git submodule add ../test.git submodule
> git reset
> git add submodule/
> 
> Now instead of just submodule gitlink there is an entry for submodule/a
> in the index.
> 
> I just thought I put this out there. Will have a look if I find the time
> to cook up a proper testcase and investigate.
> 
> Cheers Heiko

This has been the case as far as I can remember, and is basically lore
in the #git irc channel).

This can also be reproduced by just cloning a repo inside another repo
and running `git add path/`.

Re: BUG: No way to set fsck. when cloning

[Kevin Daudt]

On Thu, May 24, 2018 at 05:25:29PM +0200, Ævar Arnfjörð Bjarmason wrote:
> When I do:
> 
> git -c fetch.fsckObjects=true clone 
> g...@github.com:robbyrussell/oh-my-zsh.git
> 
> I get:
> 
> error: object 2b7227859263b6aabcc28355b0b994995b7148b6: 
> zeroPaddedFilemode: contains zero-padded file modes
> fatal: Error in object
> fatal: index-pack failed
> 
> The docs (https://git-scm.com/docs/git-config#git-config-fsckltmsg-idgt)
> say you can override this with -c fsck.zeroPaddedFilemode = ignore, but
> I see in builtin/fsck.c that just fsck_config() knows about this, and
> indeed this works *after* you clone the repo when you use 'git fsck'.
> 
> I don't have time to fix this now, but what's the best approach here?
> Make all the relevant commands copy what fsck_config() is doing, or
> should fsck_object() be lazily looking up this config by itself?

Apparently someone reported this earlier[0]. Johannes replied:

>  Well, you can apparently have your cake and eat it too (see
> https://git-scm.com/docs/git-config#git-config-receivefsckltmsg-idgt):
> 
> receive.fsck.::
> When `receive.fsckObjects` is set to true, errors can be switched
> to warnings and vice versa by configuring the `receive.fsck.`
> setting where the `` is the fsck message ID and the value
> is one of `error`, `warn` or `ignore`. For convenience, fsck prefixes
> the error/warning with the message ID, e.g. "missingEmail: invalid
> author/committer line - missing email" means that setting
> `receive.fsck.missingEmail = ignore` will hide that issue.
> 
> In your case, use receive.fsck.zeroPaddedFilemode=ignore=warn (or
> =ignore).

[0]https://public-inbox.org/git/alpine.deb.2.21.1.1801042125430...@minint-6bku6qn.europe.corp.microsoft.com/

Hope this helps, Kevin.

Re: Git push error due to hooks error

[Kevin Daudt]

On Mon, May 14, 2018 at 05:44:06PM +, Barodia, Anjali (Nokia - IN/Noida) 
wrote:
> Hi Support,
> 
> 
> I was trying to push local git to another git on gerrit, but stuck with an 
> hook error.
> This is a very large repo and while running the command "git push origin 
> --all"
> I am getting this errors:
> 
> remote: (W) 92e19d4: too many commit message lines longer than 70 characters; 
> manually wrap lines
> remote: (W) de2245b: too many commit message lines longer than 70 characters; 
> manually wrap lines
> remote: (W) dc6e982: too many commit message lines longer than 70 characters; 
> manually wrap lines
> remote: (W) d2e2efd: too many commit message lines longer than 70 characters; 
> manually wrap lines
> remote: error: internal error while processing changes To 
> ssh_url_path:8282/SI_VF.git
>  ! [remote rejected]   master -> master (Error running hook 
> /opt/gerrit/hooks/ref-update)
> error: failed to push some refs to 'ssh_user@url_path:8282/SI_VF.git'
> 
> I tried to push after deleting the .git/hooks, but still get the same error.
> Please help me in fixing this error.
> 
> Thanks with Regards,
> Anjali Barodia

Did you remove the hook from the remote repo, or the local repo? Because
this is a hook which runs on the repo you are pushing too. Something
like pre-receive or pre-update is causing this.

Kevin

Re: [PATCH 1/3] checkout.c: add strict usage of -- before file_path

[Kevin Daudt]

On Sat, May 12, 2018 at 08:23:32PM -0600, Dannier Castro L wrote:
> Currently,  is a complex command able to handle both
> branches and files without any distintion other than their names,
> taking into account that depending on the type (branch or file),
> the functionality is completely different, the easier example:
> 
> $ git checkout   # Switch from current branch to .
> $ git checkout # Restore  from HEAD, discarding
>  # changes if it's necessary.
> $ git checkout --  # The same as the last one, only with an
>  # useless '--'.
> 
> For GIT new users, this complicated versatility of  could
> be very confused, also considering that actually the flag '--' is
> completely useless (added or not, there is not any difference for
> this command), when the same program messages promote the use of
> this flag.
> 
> Regarding the 's power to overwrite any file, discarding
> changes if they exist without some way of recovering them, the
> solution propuses that the usage of '--' is strict before to
> specify the file(s) path(s) for any  command (including
> all types of flags), as a "defense barrier" to make sure about
> user's knowledge and intension running .
> 
> The solution consists in detect '--' into command args, allowing
> the discard of changes and considering the following names as
> file paths, otherwise, they are branch names.
> 
> Signed-off-by: Dannier Castro L 

One data point indicating this is giving issues is that today on IRC a
user was confused why `git checkout pt` did not show any message and did
not checkout a remote branch called 'pt' as they expected. It turned out
they also had a local file/dir called 'pt', which caused git to checkout
that file/dir rather than creating a local branch based on the remote
branch.

Kevin

Re: Git enhancement request - checkout (clone) set modified dates to commit date

[Kevin Daudt]

On Sun, Apr 22, 2018 at 03:01:10PM -0400, Andrew Wolfe wrote:
> Hi Brian,
> 
> Not completely sure what you're saying.  If the files on master are
> not changed, the changed files' commit timestamps will be older than
> the branch commit timestamps.
> 
> However, if I check out master after committing to a branch, the
> modifications will necessarily disappear because they haven't been
> committed to master.  Instead, under my proposal, each will get the
> timestamp of its prior commit.
> 

Say I build the project while on a certain branch. Then I checkout a
different branch and build again. You would expect that the targets of
every source file that have changed are rebuilt.

When you would restore the timestamp back to the commit timestamp, the
timestamp will be older then the target, and make will not rebuild it,
leaving you with outdated build targets.

Re: "git branch" issue in 2.16.1

[Kevin Daudt]

On Thu, Feb 08, 2018 at 12:27:07PM +0100, Lars Schneider wrote:
> 
> > On 08 Feb 2018, at 12:13, Lars Schneider  wrote:
> > 
> > 
> >> On 08 Feb 2018, at 09:50, Jeff King  wrote:
> >> 
> >> On Wed, Feb 07, 2018 at 11:20:08PM +0100, Lars Schneider wrote:
> >> 
>  1. You have $LESS in your environment (without "F") on one platform
>    but not the other.
> >>> 
> >>> I think that's it. On my system LESS is defined to "-R".
> >>> 
> >>> This opens the pager:
> >>> 
> >>>   $ echo "TEST" | less
> >>> 
> >>> This does not open the pager:
> >>> 
> >>>   $ echo "TEST" | less -FRX
> >>> 
> >>> That means "F" works on macOS but Git doesn't set it because LESS is
> >>> already in my environment.
> >>> 
> >>> Question is, why is LESS set that way on my system? I can't find
> >>> it in .bashrc .bash_profile .zshrc and friends.
> >> 
> >> There's also /etc/bash.bashrc, /etc/profile, etc. I don't know what's
> >> normal in the mac world. You can try running:
> >> 
> >> bash -ix 2>&1  >> 
> >> to see what your startup code is doing. I don't know of a good way to
> >> correlate that with the source files, though. Or even to ask bash which
> >> startup files it's looking in.
> > 
> > Unfortunately, this command doesn't work for me.
> > 
> > I ask around and most of my coworkers have LESS="-R".
> > Only the coworker that doesn't really use his Mac and has
> > no customizations does not have $LESS defined.
> > 
> > Therefore, I think it is likely some third party component
> > that sets $LESS.
> > 
> > @Jason:
> > Do you have homebrew, iTerm2, and/or oh-my-zsh installed?
> 
> Ha. I found it it! It is indeed oh-my-zsh:
> https://github.com/robbyrussell/oh-my-zsh/blob/master/lib/misc.zsh#L23
> 
> Let's see if oh-my-zsh is willing to change that...
> 
> - Lars

I've just added unset LESS in my .zshrc, but for most users it would be
better if they don't set it at all.

Re: [BUG] git remote prune removes local tags, depending on fetch config

[Kevin Daudt]

On Tue, Jan 16, 2018 at 12:48:32PM +0100, Andreas Schwab wrote:
> On Jan 15 2018, Michael Giuffrida  wrote:
> 
> > It doesn't seem like a useful feature -- you wouldn't expect `git
> > fetch --prune` to remove your local branches that you were developing
> > on, right?
> 
> Don't mix local and remote refs.  There is a reason why remote tracking
> branches are put in a separate name space.  If you fetch the remote tags
> into a separate name space (eg. refs/remote/tags/*:refs/tags/*) then
> there is no conflict.
> 
> Andreas.

But then they are no longer considered tags, but remote tracking
branches. Tools like git tag and git describe won't consider them, and
git branch -r would show them as remote tracking branches.

> 
> -- 
> Andreas Schwab, sch...@linux-m68k.org
> GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
> "And now for something completely different."

Re: [PATCH] Add shell completion for git remote rm

[Kevin Daudt]

On Wed, Jan 17, 2018 at 07:44:19AM +0700, Duy Nguyen wrote:
> On Tue, Jan 16, 2018 at 10:57:34AM -0800, Junio C Hamano wrote:
> > Duy Nguyen  writes:
> > 
> > > On Tue, Jan 16, 2018 at 4:43 AM, Keith Smiley  wrote:
> > >> So it sounds like either we should deprecate rm, or I should update the 
> > >> patch to the suggested method where we just complete remotes, but not rm 
> > >> in the list of completions.
> > >
> > > I vote for deprecation. I could send a patch to start warning to
> > > switch from 'rm' to 'remove'. Then perhaps we can delete it after two
> > > releases. It's not classified as plumbing should we don't have worry
> > > too much about scripts relying on 'remote rm'.
> > 
> > I do not know about "two releases" part (which amounts to merely
> > 20-24 weeks), but looking at "git remote -h" output and seeing that
> > we do spell out "rename" (instead of saying "mv" or something cryptic),
> > it probably makes sense to remove "rm" some time in the future.
> > 
> > I actually do think "rm" is _already_ deprecated.  
> > 
> > "git remote --help" does not mention it in its synopsis section and
> > it merely has ", rm" after "remove" as if an afterthought.
> 
> It's actually my bad. I should have replaced 'rm' with 'remove' in
> git-remote.txt in e17dba8fe1 (remote: prefer subcommand name 'remove'
> to 'rm' - 2012-09-06)
> 
> > I am not sure if it is worth being more explicit, perhaps like this?
> 
> Looks good. If we want to be more careful, we can follow up with
> something even more annoying like this before removing 'rm'
> 
> -- 8< --
> diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
> index 577b969c1b..0a544703e6 100644
> --- a/Documentation/git-remote.txt
> +++ b/Documentation/git-remote.txt
> @@ -90,7 +90,6 @@ In case  and  are the same, and  is a file 
> under
>  the configuration file format.
>  
>  'remove'::
> -'rm'::
>  
>  Remove the remote named . All remote-tracking branches and
>  configuration settings for the remote are removed.
> diff --git a/builtin/remote.c b/builtin/remote.c
> index d95bf904c3..774ef6931e 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -1609,7 +1609,10 @@ int cmd_remote(int argc, const char **argv, const char 
> *prefix)
>   result = add(argc, argv);
>   else if (!strcmp(argv[0], "rename"))
>   result = mv(argc, argv);
> - else if (!strcmp(argv[0], "rm") || !strcmp(argv[0], "remove"))
> + else if (!strcmp(argv[0], "rm")) {
> + warning(_("'rm' is a deprecated synonym. Use 'remove' 
> instead."));
> + result = rm(argc, argv);
> + } else if (!strcmp(argv[0], "remove"))
>   result = rm(argc, argv);
>   else if (!strcmp(argv[0], "set-head"))
>   result = set_head(argc, argv);
> -- 8< --
> 
> PS. This also makes me thing about supporting subcommand aliases, so
> that people can add back 'git remote rm' if they like (or something
> like 'git r rm' when they alias 'remote' as well). Which is probably a
> good thing to do. Long command names are fine when you have completion
> support, they are a pain to type otherwise.
> 

Couldn't they just create an alias like git rrm then, if they use it so
often that it becomes an issue? Having another layer of aliases does
create a lot of complexity.

> --
> Duy

Re: [PATCH] sha1_file: remove static strbuf from sha1_file_name()

[Kevin Daudt]

On Tue, Jan 16, 2018 at 08:18:14AM +0100, Christian Couder wrote:
> Using a static buffer in sha1_file_name() is error prone
> and the performance improvements it gives are not needed
> in most of the callers.
> 
> So let's get rid of this static buffer and, if necessary
> or helpful, let's use one in the caller.

Missing sign-off

> ---
>  cache.h   |  8 +++-
>  http-walker.c |  6 --
>  http.c| 16 ++--
>  sha1_file.c   | 38 +-
>  4 files changed, 42 insertions(+), 26 deletions(-)
> 
> diff --git a/cache.h b/cache.h
> index d8b975a571..6db565408e 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -957,12 +957,10 @@ extern void check_repository_format(void);
>  #define TYPE_CHANGED0x0040
>  
>  /*
> - * Return the name of the file in the local object database that would
> - * be used to store a loose object with the specified sha1.  The
> - * return value is a pointer to a statically allocated buffer that is
> - * overwritten each time the function is called.
> + * Put in `buf` the name of the file in the local object database that
> + * would be used to store a loose object with the specified sha1.
>   */
> -extern const char *sha1_file_name(const unsigned char *sha1);
> +extern void sha1_file_name(struct strbuf *buf, const unsigned char *sha1);
>  
>  /*
>   * Return an abbreviated sha1 unique within this repository's object 
> database.
> diff --git a/http-walker.c b/http-walker.c
> index 1ae8363de2..07c2b1af82 100644
> --- a/http-walker.c
> +++ b/http-walker.c
> @@ -544,8 +544,10 @@ static int fetch_object(struct walker *walker, unsigned 
> char *sha1)
>   } else if (hashcmp(obj_req->sha1, req->real_sha1)) {
>   ret = error("File %s has bad hash", hex);
>   } else if (req->rename < 0) {
> - ret = error("unable to write sha1 filename %s",
> - sha1_file_name(req->sha1));
> + struct strbuf buf = STRBUF_INIT;
> + sha1_file_name(, req->sha1);
> + ret = error("unable to write sha1 filename %s", buf.buf);
> + strbuf_release();
>   }
>  
>   release_http_object_request(req);
> diff --git a/http.c b/http.c
> index 5977712712..5979305bc9 100644
> --- a/http.c
> +++ b/http.c
> @@ -2168,7 +2168,7 @@ struct http_object_request 
> *new_http_object_request(const char *base_url,
>   unsigned char *sha1)
>  {
>   char *hex = sha1_to_hex(sha1);
> - const char *filename;
> + struct strbuf filename = STRBUF_INIT;
>   char prevfile[PATH_MAX];
>   int prevlocal;
>   char prev_buf[PREV_BUF_SIZE];
> @@ -2180,14 +2180,15 @@ struct http_object_request 
> *new_http_object_request(const char *base_url,
>   hashcpy(freq->sha1, sha1);
>   freq->localfile = -1;
>  
> - filename = sha1_file_name(sha1);
> + sha1_file_name(, sha1);
>   snprintf(freq->tmpfile, sizeof(freq->tmpfile),
> -  "%s.temp", filename);
> +  "%s.temp", filename.buf);
>  
> - snprintf(prevfile, sizeof(prevfile), "%s.prev", filename);
> + snprintf(prevfile, sizeof(prevfile), "%s.prev", filename.buf);
>   unlink_or_warn(prevfile);
>   rename(freq->tmpfile, prevfile);
>   unlink_or_warn(freq->tmpfile);
> + strbuf_release();
>  
>   if (freq->localfile != -1)
>   error("fd leakage in start: %d", freq->localfile);
> @@ -2302,6 +2303,7 @@ void process_http_object_request(struct 
> http_object_request *freq)
>  int finish_http_object_request(struct http_object_request *freq)
>  {
>   struct stat st;
> + struct strbuf filename = STRBUF_INIT;
>  
>   close(freq->localfile);
>   freq->localfile = -1;
> @@ -2327,8 +2329,10 @@ int finish_http_object_request(struct 
> http_object_request *freq)
>   unlink_or_warn(freq->tmpfile);
>   return -1;
>   }
> - freq->rename =
> - finalize_object_file(freq->tmpfile, sha1_file_name(freq->sha1));
> +
> + sha1_file_name(, freq->sha1);
> + freq->rename = finalize_object_file(freq->tmpfile, filename.buf);
> + strbuf_release();
>  
>   return freq->rename;
>  }
> diff --git a/sha1_file.c b/sha1_file.c
> index 3da70ac650..f66c21b2da 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -321,15 +321,11 @@ static void fill_sha1_path(struct strbuf *buf, const 
> unsigned char *sha1)
>   }
>  }
>  
> -const char *sha1_file_name(const unsigned char *sha1)
> +void sha1_file_name(struct strbuf *buf, const unsigned char *sha1)
>  {
> - static struct strbuf buf = STRBUF_INIT;
> -
> - strbuf_reset();
> - strbuf_addf(, "%s/", get_object_directory());
> + strbuf_addf(buf, "%s/", get_object_directory());
>  
> - fill_sha1_path(, sha1);
> - return buf.buf;
> + fill_sha1_path(buf, sha1);
>  }
>  
>  struct strbuf *alt_scratch_buf(struct alternate_object_database *alt)
> @@ -710,7 +706,12 @@ int check_and_freshen_file(const char *fn, int freshen)
>  
>  static int 

Re: [PATCH] Add shell completion for git remote rm

[Kevin Daudt]

On Fri, Dec 29, 2017 at 02:01:00AM +, Keith Smiley wrote:
> From: Keith Smiley 
> 
> Previously git remote rm did not complete your list of removes as remove
> does.

Your signed-off-by[1] is missing, could you please add that?

[1]:
https://github.com/git/git/blob/master/Documentation/SubmittingPatches#L278

> ---
>  contrib/completion/git-completion.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 3683c772c5586..3e9044087e6ba 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2665,7 +2665,7 @@ _git_config ()
>  _git_remote ()
>  {
>   local subcommands="
> - add rename remove set-head set-branches
> + add rename remove rm set-head set-branches
>   get-url set-url show prune update
>   "
>   local subcommand="$(__git_find_on_cmdline "$subcommands")"
> 
> --
> https://github.com/git/git/pull/448

Re: [PATCH] setup.c: move statement under condition

[Kevin Daudt]

On Sun, Dec 24, 2017 at 12:15:35PM +0400, Vadim Petrov wrote:
> Thank you for your replay.
> 
> > I have to be honest: this commit message (including the subject) left me
> > quite puzzled as to the intent of this patch.
> 
> I still only learn English and correctly express my thoughts while somewhat 
> difficult.
> 
> > If you also have a background story that motivated you to work on this
> > patch (for example, if you hit a huge performance bottleneck with some
> > tool that fed thousands of absolute paths to Git that needed to be turned
> > into paths relative to the worktree's top-level directory), I would
> > definitely put that into the commit message, too, if I were you.
> 
> I have no such reason. I just saw it and wanted to change it.

A commit message contains the reason why this is a good change to make.
It lets others know what problems it's trying to solve or what usecase
it tries to satisfy.

The commit message basically needs to convince others why the change is
necessary / desired, now, and in the future. 

This will help others to follow your thought process and it gives you
the possiblity to communicate trade-offs you made, all which cannot
inferred from the patch.

For simple changes, the motivation can be simple too.

To make it concrete: You are talking about a condition. What condition?
And you say that the previously obtained value will not be necessary.
What do you do with that value then? Why does this change improve the
situation? 

These are things you can state in your commit message.

Hope this helps, Kevin

> > Up until recently, we encouraged dropping the curly brackets from
> > single-line statements, but apparently that changed. It is now no longer
> > clear, and often left to the taste of the contributor. But not always.
> > Sometimes we start a beautiful thread discussion the pros and cons of
> > curly brackets in the middle of patch review, and drop altogether
> > reviewing the actual patch.
> 
> I was guided by the rule from the Documentation/CodingGuidelines:
>   When there are multiple arms to a conditional and some of them
>   require braces, enclose even a single line block in braces for
>   consistency.
> And other code from setup.c:
>   from function get_common_dir:
>   if (!has_common) {
>   /* several commands */
>   } else {
>   free(candidate->work_tree);
>   }
>   from function get_common_dir_noenv:
>   if (file_exists(path.buf)) {
>   /* several commands */
>   } else {
>   strbuf_addstr(sb, gitdir);
>   }
> 
> > In short: I think your patch does the right thing, and I hope that you
> > find my suggestions to improve the patch useful.
> 
> I fixed the patch according to your suggestions.
> 
> 
> Signed-off-by: Vadim Petrov 
> ---
>  setup.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/setup.c b/setup.c
> index 8cc34186c..1a414c256 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -27,26 +27,26 @@ static int abspath_part_inside_repo(char *path)
>  {
>   size_t len;
>   size_t wtlen;
>   char *path0;
>   int off;
>   const char *work_tree = get_git_work_tree();
>  
>   if (!work_tree)
>   return -1;
>   wtlen = strlen(work_tree);
>   len = strlen(path);
> - off = offset_1st_component(path);
>  
> - /* check if work tree is already the prefix */
> - if (wtlen <= len && !strncmp(path, work_tree, wtlen)) {
> + if (wtlen > len || strncmp(path, work_tree, wtlen))
> + off = offset_1st_component(path);
> + else { /* check if work tree is already the prefix */
>   if (path[wtlen] == '/') {
>   memmove(path, path + wtlen + 1, len - wtlen);
>   return 0;
>   } else if (path[wtlen - 1] == '/' || path[wtlen] == '\0') {
>   /* work tree is the root, or the whole path */
>   memmove(path, path + wtlen, len - wtlen + 1);
>   return 0;
>   }
>   /* work tree might match beginning of a symlink to work tree */
>   off = wtlen;
>   }
> -- 
> 2.15.1.433.g936d1b989

Re: [PATCH v2 0/5] SHA1DC fixes & fully moving to a git.git submodule

[Kevin Daudt]

On Fri, Dec 08, 2017 at 10:29:56PM +, Ævar Arnfjörð Bjarmason wrote:
> Here's v2 as promised. Comments per-patch.
> 
>   sha1dc: remove in favor of using sha1collisiondetection as a submodule
> 
> Reword & expand commit message.

Is this commit missing from the mailing list because the e-mail is too
large?

Re: [PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default

[Kevin Daudt]

On Sat, Dec 09, 2017 at 01:30:14PM +0100, Kevin Daudt wrote:
> On Tue, Dec 05, 2017 at 02:02:50AM -0500, Jeff King wrote:
> > On Tue, Nov 28, 2017 at 09:32:13PM +, Ævar Arnfjörð Bjarmason wrote:
> > 
> > > Change the build process so that instead of needing to supply
> > > DC_SHA1_SUBMODULE=YesPlease to use the sha1collisiondetection
> > > submodule instead of the copy of the same code shipped in the sha1dc
> > > directory, it uses the submodule by default unless
> > > NO_DC_SHA1_SUBMODULE=UnfortunatelyYes is supplied.
> > > 
> > > This reverses the logic added by me in 86cfd61e6b ("sha1dc: optionally
> > > use sha1collisiondetection as a submodule", 2017-07-01). Git has now
> > > shipped with the submodule in git.git for two major releases, if we're
> > > ever going to migrate to fully using it instead of perpetually
> > > maintaining both sha1collisiondetection and the sha1dc directory this
> > > is a logical first step.
> > > 
> > > This change removes the "auto" logic Junio added in
> > > cac87dc01d ("sha1collisiondetection: automatically enable when
> > > submodule is populated", 2017-07-01), I feel that automatically
> > > falling back to using sha1dc would defeat the point, which is to smoke
> > > out any remaining users of git.git who have issues cloning the
> > > submodule for whatever reason.
> > 
> > I'm not sure how I feel about this. I see your point that there's no
> > real value in maintaining two systems indefinitely.  At the same time, I
> > wonder how much value the submodule strategy is actually bringing us.
> > 
> > IOW, are we agreed that the path forward is to get everybody using the
> > submodule?
> > 
> > It seems like it's going to cause some minor pain for CI and packaging
> > systems that now need to care about submodules (so at least flipping the
> > switch, but maybe also dealing with having a network dependency for the
> > build that was not already there).
> > 
> > I'll admit I'm more sensitive to this than most people, since I happen
> > to maintain a fork of Git that I run through an internal CI system. And
> > that CI otherwise depends only on the locally-held fork, not any
> > external resources. But I'm probably in a fairly unique situation there.
> > 
> > -Peff
> 
> To add to this point, package systems such as Alpinelinux and Archlinux
> (and probably others) work with released tarballs, not cloned
> repositories. For them, there is no easy way to get the submodule
> contents (the release tarballs would not contain it).
> 
> Once we would switch over to submodules only (because we do not want to
> maintain 2 separate systems), it would be a lot of hassle for those
> projects to get the sha1collisiondetection contents.
> 
> That's in my opinion a bigger disadvantage of submodules, commands like
> git archive do not support it, making it harder to get self-contained
> tarballs.
> 
> Perpahs there is a good solution to that problem, but then I'd like to
> hear it.
> 
> Kevin.

I missed the v2 Ævar sent. I see that there `make dist` is adjusted to
include sha1collisiondetection, so that would at least solve this
problem.

Re: [PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default

[Kevin Daudt]

On Tue, Dec 05, 2017 at 02:02:50AM -0500, Jeff King wrote:
> On Tue, Nov 28, 2017 at 09:32:13PM +, Ævar Arnfjörð Bjarmason wrote:
> 
> > Change the build process so that instead of needing to supply
> > DC_SHA1_SUBMODULE=YesPlease to use the sha1collisiondetection
> > submodule instead of the copy of the same code shipped in the sha1dc
> > directory, it uses the submodule by default unless
> > NO_DC_SHA1_SUBMODULE=UnfortunatelyYes is supplied.
> > 
> > This reverses the logic added by me in 86cfd61e6b ("sha1dc: optionally
> > use sha1collisiondetection as a submodule", 2017-07-01). Git has now
> > shipped with the submodule in git.git for two major releases, if we're
> > ever going to migrate to fully using it instead of perpetually
> > maintaining both sha1collisiondetection and the sha1dc directory this
> > is a logical first step.
> > 
> > This change removes the "auto" logic Junio added in
> > cac87dc01d ("sha1collisiondetection: automatically enable when
> > submodule is populated", 2017-07-01), I feel that automatically
> > falling back to using sha1dc would defeat the point, which is to smoke
> > out any remaining users of git.git who have issues cloning the
> > submodule for whatever reason.
> 
> I'm not sure how I feel about this. I see your point that there's no
> real value in maintaining two systems indefinitely.  At the same time, I
> wonder how much value the submodule strategy is actually bringing us.
> 
> IOW, are we agreed that the path forward is to get everybody using the
> submodule?
> 
> It seems like it's going to cause some minor pain for CI and packaging
> systems that now need to care about submodules (so at least flipping the
> switch, but maybe also dealing with having a network dependency for the
> build that was not already there).
> 
> I'll admit I'm more sensitive to this than most people, since I happen
> to maintain a fork of Git that I run through an internal CI system. And
> that CI otherwise depends only on the locally-held fork, not any
> external resources. But I'm probably in a fairly unique situation there.
> 
> -Peff

To add to this point, package systems such as Alpinelinux and Archlinux
(and probably others) work with released tarballs, not cloned
repositories. For them, there is no easy way to get the submodule
contents (the release tarballs would not contain it).

Once we would switch over to submodules only (because we do not want to
maintain 2 separate systems), it would be a lot of hassle for those
projects to get the sha1collisiondetection contents.

That's in my opinion a bigger disadvantage of submodules, commands like
git archive do not support it, making it harder to get self-contained
tarballs.

Perpahs there is a good solution to that problem, but then I'd like to
hear it.

Kevin.

Re: [PATCH 3/3] pull: add config option for verifySignatures

[Kevin Daudt]

On Sat, Dec 09, 2017 at 09:05:30AM +, Hans Jerry Illikainen wrote:
> Verify the signature of the tip commit when `pull.verifySignatures` is
> true.  This option overrides `merge.verifySignatures` on pull, and can
> be disabled with the option `--no-verify-signatures`.

Is there a reason why git pull would need a different behaviour from git
merge? Pull itself is just a convenience command for fetch +
merge/rebase.

One precedent for having a separate configuration option for pull
however is 'pull.ff', so there might be a usecase for it.

I guess your commit message could use a motivation on why you want to
set this differently from 'merge.verifySignature'.

> 
> Signed-off-by: Hans Jerry Illikainen 
> ---
>  Documentation/config.txt  |  8 
>  builtin/pull.c| 25 +
>  t/t5520-pull.sh   | 18 ++
>  t/t5573-pull-verify-signatures.sh | 32 
>  4 files changed, 83 insertions(+)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index c1598ee70..0cd2bc597 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2596,6 +2596,14 @@ pull.ff::
>   allowed (equivalent to giving the `--ff-only` option from the
>   command line). This setting overrides `merge.ff` when pulling.
>  
> +pull.verifySignatures::
> + Verify that the tip commit of the side branch being merged is
> + signed with a valid key, i.e. a key that has a valid uid: in the
> + default trust model, this means the signing key has been signed
> + by a trusted key. If the tip commit of the side branch is not
> + signed with a valid key, the merge is aborted. This setting
> + overrides `merge.verifySignatures` when pulling.
> +
>  pull.rebase::
>   When true, rebase branches on top of the fetched branch, instead
>   of merging the default branch from the default remote when "git
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 166b777ed..791365915 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -300,6 +300,28 @@ static const char *config_get_ff(void)
>  }
>  
>  /**
> + * If pull.verifySignatures is unset, returns NULL. If pull.verifySignatures 
> is
> + * "true", returns "--verify-signatures". If pull.verifySignatures is 
> "false",
> + * returns "--no-verify-signatures". Otherwise, die with an error.
> + */
> +static const char *config_get_verify_signatures(void)
> +{
> + const char *value;
> +
> + if (git_config_get_value("pull.verifysignatures", ))
> + return NULL;
> +
> + switch (git_parse_maybe_bool(value)) {
> + case 0:
> + return "--no-verify-signatures";
> + case 1:
> + return "--verify-signatures";
> + default:
> + die(_("Invalid value for pull.verifysignatures: %s"), value);
> + }
> +}
> +
> +/**
>   * Returns the default configured value for --rebase. It first looks for the
>   * value of "branch.$curr_branch.rebase", where $curr_branch is the current
>   * branch, and if HEAD is detached or the configuration key does not exist,
> @@ -849,6 +871,9 @@ int cmd_pull(int argc, const char **argv, const char 
> *prefix)
>   if (!opt_ff)
>   opt_ff = xstrdup_or_null(config_get_ff());
>  
> + if (!opt_verify_signatures)
> + opt_verify_signatures = 
> xstrdup_or_null(config_get_verify_signatures());
> +
>   if (opt_rebase < 0)
>   opt_rebase = config_get_rebase();
>  
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 59c4b778d..cdf1fd213 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -416,6 +416,15 @@ test_expect_success "pull --rebase warns on 
> --verify-signatures" '
>   test_i18ngrep "ignoring --verify-signatures for rebase" err
>  '
>  
> +test_expect_success "pull --rebase warns on pull.verifySignatures=true" '
> + test_config pull.verifySignatures true &&
> + git reset --hard before-rebase &&
> + git pull --rebase . copy 2>err &&
> + test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
> + test new = "$(git show HEAD:file2)" &&
> + test_i18ngrep "ignoring --verify-signatures for rebase" err
> +'
> +
>  test_expect_success "pull --rebase does not warn on --no-verify-signatures" '
>   git reset --hard before-rebase &&
>   git pull --rebase --no-verify-signatures . copy 2>err &&
> @@ -424,6 +433,15 @@ test_expect_success "pull --rebase does not warn on 
> --no-verify-signatures" '
>   test_i18ngrep ! "verify-signatures" err
>  '
>  
> +test_expect_success "pull --rebase does not warn on 
> pull.verifySignatures=false" '
> + test_config pull.verifySignatures false &&
> + git reset --hard before-rebase &&
> + git pull --rebase . copy 2>err &&
> + test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
> + test new = "$(git show HEAD:file2)" &&
> + test_i18ngrep ! "verify-signatures" err
> +'
> +
>  # 

Re: [PATCH 2/3] t: add tests for pull --verify-signatures

[Kevin Daudt]

On Sat, Dec 09, 2017 at 09:05:29AM +, Hans Jerry Illikainen wrote:
> Add tests for `pull --verify-signatures` with untrusted, bad and no
> signatures.  Previously the only test for `--verify-signatures` was to
> make sure that `pull --rebase --verify-signatures` result in a warning
> (t5520-pull.sh).

Nice!

Same thing regarding the `git checkout initial` commands counts
here too.

> 
> Signed-off-by: Hans Jerry Illikainen 
> ---
>  t/t5573-pull-verify-signatures.sh | 78 
> +++
>  1 file changed, 78 insertions(+)
>  create mode 100755 t/t5573-pull-verify-signatures.sh
> 
> diff --git a/t/t5573-pull-verify-signatures.sh 
> b/t/t5573-pull-verify-signatures.sh
> new file mode 100755
> index 0..700247910
> --- /dev/null
> +++ b/t/t5573-pull-verify-signatures.sh
> @@ -0,0 +1,78 @@
> +#!/bin/sh
> +
> +test_description='pull signature verification tests'
> +. ./test-lib.sh
> +. "$TEST_DIRECTORY/lib-gpg.sh"
> +
> +test_expect_success GPG 'create repositories with signed commits' '
> + echo 1 >a && git add a &&
> + test_tick && git commit -m initial &&
> + git tag initial &&
> +
> + git clone . signed &&
> + (
> + cd signed &&
> + echo 2 >b && git add b &&
> + test_tick && git commit -S -m "signed"
> + ) &&
> +
> + git clone . unsigned &&
> + (
> + cd unsigned &&
> + echo 3 >c && git add c &&
> + test_tick && git commit -m "unsigned"
> + ) &&
> +
> + git clone . bad &&
> + (
> + cd bad &&
> + echo 4 >d && git add d &&
> + test_tick && git commit -S -m "bad" &&
> + git cat-file commit HEAD >raw &&
> + sed -e "s/bad/forged bad/" raw >forged &&
> + git hash-object -w -t commit forged >forged.commit &&
> + git checkout $(cat forged.commit)
> + ) &&
> +
> + git clone . untrusted &&
> + (
> + cd untrusted &&
> + echo 5 >e && git add e &&
> + test_tick && git commit -SB7227189 -m "untrusted"
> + )
> +'
> +
> +test_expect_success GPG 'pull unsigned commit with --verify-signatures' '
> + test_must_fail git pull --ff-only --verify-signatures unsigned 
> 2>pullerror &&
> + test_i18ngrep "does not have a GPG signature" pullerror
> +'
> +
> +test_expect_success GPG 'pull commit with bad signature with 
> --verify-signatures' '
> + test_must_fail git pull --ff-only --verify-signatures bad 2>pullerror &&
> + test_i18ngrep "has a bad GPG signature" pullerror
> +'
> +
> +test_expect_success GPG 'pull commit with untrusted signature with 
> --verify-signatures' '
> + test_must_fail git pull --ff-only --verify-signatures untrusted 
> 2>pullerror &&
> + test_i18ngrep "has an untrusted GPG signature" pullerror
> +'
> +
> +test_expect_success GPG 'pull signed commit with --verify-signatures' '
> + git pull --verify-signatures signed >pulloutput &&
> + test_i18ngrep "has a good GPG signature" pulloutput &&
> + git checkout initial
> +'
> +
> +test_expect_success GPG 'pull commit with bad signature without 
> verification' '
> + git pull --ff-only bad 2>pullerror &&
> + git checkout initial
> +'
> +
> +test_expect_success GPG 'pull commit with bad signature with 
> --no-verify-signatures' '
> + test_config merge.verifySignatures true &&
> + test_config pull.verifySignatures true &&
> + git pull --ff-only --no-verify-signatures bad 2>pullerror &&
> + git checkout initial
> +'
> +
> +test_done
> -- 
> 2.11.0
> 

Re: [PATCH 1/3] merge: add config option for verifySignatures

[Kevin Daudt]

Hello Hans Jerry,

Thank you for your contribution. I have soem remarks below.

On Sat, Dec 09, 2017 at 09:05:28AM +, Hans Jerry Illikainen wrote:
> Verify the signature of the tip commit when `merge.verifySignatures` is
> true.  This can be overridden with `--no-verify-signatures`.
> 
> Signed-off-by: Hans Jerry Illikainen 

I miss the motivation in the commit message. I imagine something like:

git merge --verify-signatures can be used to verify that the tip
commit of the branch being merged in is properly signed, but it's
cumbersome to have to specify that every time.

Add a configuration option that enables this behaviour by default,
which can be overridden by --no-verify-signatures.


> ---
>  Documentation/merge-config.txt |  7 +++
>  builtin/merge.c|  2 ++
>  t/t7612-merge-verify-signatures.sh | 43 
> --
>  3 files changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
> index df3ea3779..76571cd3b 100644
> --- a/Documentation/merge-config.txt
> +++ b/Documentation/merge-config.txt
> @@ -26,6 +26,13 @@ merge.ff::
>   allowed (equivalent to giving the `--ff-only` option from the
>   command line).
>  
> +merge.verifySignatures::
> + Verify that the tip commit of the side branch being merged is
> + signed with a valid key, i.e. a key that has a valid uid: in the
> + default trust model, this means the signing key has been signed
> + by a trusted key. If the tip commit of the side branch is not
> + signed with a valid key, the merge is aborted.
> +

This is a verbatim copy of the explenation of --verify-signatures. Would
it be an idea to refer to the explenation of merge --verify-signatures?

>  include::fmt-merge-msg-config.txt[]
>  
>  merge.renameLimit::
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 612dd7bfb..30264cfd7 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -567,6 +567,8 @@ static int git_merge_config(const char *k, const char *v, 
> void *cb)
>  
>   if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
>   show_diffstat = git_config_bool(k, v);
> + else if (!strcmp(k, "merge.verifysignatures"))
> + verify_signatures = git_config_bool(k, v);
>   else if (!strcmp(k, "pull.twohead"))
>   return git_config_string(_twohead, k, v);
>   else if (!strcmp(k, "pull.octopus"))
> diff --git a/t/t7612-merge-verify-signatures.sh 
> b/t/t7612-merge-verify-signatures.sh
> index 8ae69a61c..f1a74a683 100755
> --- a/t/t7612-merge-verify-signatures.sh
> +++ b/t/t7612-merge-verify-signatures.sh
> @@ -39,23 +39,62 @@ test_expect_success GPG 'merge unsigned commit with 
> verification' '
>   test_i18ngrep "does not have a GPG signature" mergeerror
>  '
>  
> +test_expect_success GPG 'merge unsigned commit with 
> merge.verifySignatures=true' '
> + test_config merge.verifySignatures true &&
> + test_must_fail git merge --ff-only side-unsigned 2>mergeerror &&
> + test_i18ngrep "does not have a GPG signature" mergeerror
> +'
> +
>  test_expect_success GPG 'merge commit with bad signature with verification' '
>   test_must_fail git merge --ff-only --verify-signatures $(cat 
> forged.commit) 2>mergeerror &&
>   test_i18ngrep "has a bad GPG signature" mergeerror
>  '
>  
> +test_expect_success GPG 'merge commit with bad signature with 
> merge.verifySignatures=true' '
> + test_config merge.verifySignatures true &&
> + test_must_fail git merge --ff-only $(cat forged.commit) 2>mergeerror &&
> + test_i18ngrep "has a bad GPG signature" mergeerror
> +'
> +
>  test_expect_success GPG 'merge commit with untrusted signature with 
> verification' '
>   test_must_fail git merge --ff-only --verify-signatures side-untrusted 
> 2>mergeerror &&
>   test_i18ngrep "has an untrusted GPG signature" mergeerror
>  '
>  
> +test_expect_success GPG 'merge commit with untrusted signature with 
> merge.verifySignatures=true' '
> + test_config merge.verifySignatures true &&
> + test_must_fail git merge --ff-only side-untrusted 2>mergeerror &&
> + test_i18ngrep "has an untrusted GPG signature" mergeerror
> +'
> +
>  test_expect_success GPG 'merge signed commit with verification' '
>   git merge --verbose --ff-only --verify-signatures side-signed 
> >mergeoutput &&
> - test_i18ngrep "has a good GPG signature" mergeoutput
> + test_i18ngrep "has a good GPG signature" mergeoutput &&
> + git checkout initial

This looks like a clean up step. If so, it's better to add
`test_when_finished 'git checkout initial'` at the beginning to clearly
mark it as a clean up step and make sure it's run even when the test
fails. Same counts for the other occurances.

> +'
> +
> +test_expect_success GPG 'merge signed commit with 
> merge.verifySignatures=true' '
> + test_config merge.verifySignatures 

Re: [PATCH v2] gitcli: tweak "man gitcli" for clarity

[Kevin Daudt]

On Thu, Nov 23, 2017 at 08:51:55AM -0500, Jeff King wrote:
> On Thu, Nov 23, 2017 at 02:45:44AM -0500, Robert P. J. Day wrote:
> 
> > > It's pretty clear to me as it is, but maybe we can write it differently.
> > > Like:
> > >
> > >   Without a disambiguating `--`, Git makes a reasonable guess. If it
> > >   cannot guess (because your request is ambiguous), then it will error
> > >   out.
> > 
> >   ok, i'll give this another try, given that there are two independent
> > points to be made here:
> > 
> > 1) even without the "--", git can generally parse the command and do
> > the right thing (or do a *valid* thing, given its heuristics)
> > 
> > 2) occasionally, without the "--", the command is really and truly
> > ambiguous, at which point git will fail and tell you to disambiguate
> > 
> >   not the wording i will use, but can we agree that those are the two
> > points to be made here?
> 
> Yep, I think so.
> 
> -Peff

Just for completeness, as it is somewhat covered by point 1 already, but
there are cases where there is no real ambiguity but you are required to
add '--' to tell git that it should not look for the file in the working
tree:

  $ git show abc123 deleted_file.txt
  fatal: ambiguous argument 'deleted_file.txt':
  unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions, like this:
  'git  [...] -- [...]'

There might be good reasons why this is, but I don't consider this to be
actually ambiguous: there is no branch called 'deleted_file.txt' and git
could know that the files exists in the mentioned commit, so it should
be pretty clear what is meant.

Might be worth documenting this.

Kevin

Re: [PATCH v2] gitcli: tweak "man gitcli" for clarity

[Kevin Daudt]

On Wed, Nov 22, 2017 at 06:19:23AM -0500, Robert P. J. Day wrote:
> On Wed, 22 Nov 2017, Junio C Hamano wrote:
> 
> > "Robert P. J. Day"  writes:
> >
> > > git repo with a file called "Gemfile", so i created a branch called
> > > "Gemfile", and when i ran:
> > >
> > >   $ git checkout Gemfile
> > >
> > > git switched to the branch. so even with the ambiguity, git
> > > obviously has some sort of precedence order it checks. so what are
> > > the rules here?
> >
> > 31b83f36 ("Merge branch 'nd/checkout-disambiguation'", 2016-09-26)
> > should have made it clear that the "checkout" command has a
> > convenience special case.
> 
>   ok, then i'm still curious about git examples that actually fail due
> to an inability to disambiguate.
> 
> rday

Here is an example with git diff

$ git init git-disambiguate
$ cd git-disambiguate
$ echo 1 >foo && git add foo && git commit -m foo
$ git branch foo
$ echo 2 >>foo && git add foo && git commit -m foo2
$ echo 3 >>foo

$ git diff foo
fatal: ambiguous argument 'foo': both revision and filename
Use '--' to separate paths from revisions, like this:
'git  [...] -- [...]'

$ git diff HEAD foo
fatal: ambiguous argument 'foo': both revision and filename
Use '--' to separate paths from revisions, like this:
'git  [...] -- [...]'

$ git diff HEAD -- foo
diff --git a/foo b/foo
index 1191247..01e79c3 100644
--- a/foo
+++ b/foo
@@ -1,2 +1,3 @@
 1
 2
+3

$ git diff HEAD foo --
diff --git a/foo b/foo
index 1191247..d00491f 100644
--- a/foo
+++ b/foo
@@ -1,2 +1 @@
 1
-2
 

Re: [PATCH v2] gitcli: tweak "man gitcli" for clarity

[Kevin Daudt]

On Tue, Nov 21, 2017 at 04:47:42PM -0500, Robert P. J. Day wrote:
> On Tue, 21 Nov 2017, Kevin Daudt wrote:
> 
> > On Tue, Nov 21, 2017 at 04:27:59PM -0500, Robert P. J. Day wrote:
> > > No major changes, just some rewording and showing some variations of
> > > general Git commands.
> > >
> > > Signed-off-by: Robert P. J. Day <rpj...@crashcourse.ca>
> > >
> > > ---
> > >
> > > diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt
> > > index 9f13266a6..d690d1ff0 100644
> > > --- a/Documentation/gitcli.txt
> > > +++ b/Documentation/gitcli.txt
> > > @@ -13,7 +13,7 @@ gitcli
> > >  DESCRIPTION
> > >  ---
> > >
> > > -This manual describes the convention used throughout Git CLI.
> > > +This manual describes the conventions used throughout Git CLI.
> > >
> > >  Many commands take revisions (most often "commits", but sometimes
> > >  "tree-ish", depending on the context and command) and paths as their
> > > @@ -32,32 +32,35 @@ arguments.  Here are the rules:
> > > between the HEAD commit and the work tree as a whole".  You can say
> > > `git diff HEAD --` to ask for the latter.
> > >
> > > - * Without disambiguating `--`, Git makes a reasonable guess, but errors
> > > -   out and asking you to disambiguate when ambiguous.  E.g. if you have a
> > > + * Without a disambiguating `--`, Git makes a reasonable guess, but can
> > > +   error out, asking you to disambiguate when ambiguous.  E.g. if you 
> > > have a
> >
> > 'Can' error out implies that it sometimes would not error out when
> > there is ambiguity. Are there situation where git does not error out
> > in that case?
> 
>   i would say (based on my limited knowledge) that if the heuristic
> kicks in and works fine, then things will work. i think it's fair to
> say that git "can" error out if the heuristic fails.
> 
> rday

In most cases that I'm aware of, you have to be explicit. If for example
you want to refer to a file that's not in the working tree, you have to
use '--'.  Even with heuristics, it would still have to error out when
it's ambiguous what the user meant.

So the way you worded it implies that there are situations where git
knows there are multiple things the user could have meant, but it would
not error out in that case.

Kevin

Re: [PATCH v2] gitcli: tweak "man gitcli" for clarity

[Kevin Daudt]

On Tue, Nov 21, 2017 at 04:27:59PM -0500, Robert P. J. Day wrote:
> No major changes, just some rewording and showing some variations of
> general Git commands.
> 
> Signed-off-by: Robert P. J. Day 
> 
> ---
> 
> diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt
> index 9f13266a6..d690d1ff0 100644
> --- a/Documentation/gitcli.txt
> +++ b/Documentation/gitcli.txt
> @@ -13,7 +13,7 @@ gitcli
>  DESCRIPTION
>  ---
> 
> -This manual describes the convention used throughout Git CLI.
> +This manual describes the conventions used throughout Git CLI.
> 
>  Many commands take revisions (most often "commits", but sometimes
>  "tree-ish", depending on the context and command) and paths as their
> @@ -32,32 +32,35 @@ arguments.  Here are the rules:
> between the HEAD commit and the work tree as a whole".  You can say
> `git diff HEAD --` to ask for the latter.
> 
> - * Without disambiguating `--`, Git makes a reasonable guess, but errors
> -   out and asking you to disambiguate when ambiguous.  E.g. if you have a
> + * Without a disambiguating `--`, Git makes a reasonable guess, but can
> +   error out, asking you to disambiguate when ambiguous.  E.g. if you have a

'Can' error out implies that it sometimes would not error out when there
is ambiguity. Are there situation where git does not error out in that
case?

> file called HEAD in your work tree, `git diff HEAD` is ambiguous, and
> you have to say either `git diff HEAD --` or `git diff -- HEAD` to
> disambiguate.
>  +
>  When writing a script that is expected to handle random user-input, it is
>  a good practice to make it explicit which arguments are which by placing
> -disambiguating `--` at appropriate places.
> +a disambiguating `--` at appropriate places.
> 
>   * Many commands allow wildcards in paths, but you need to protect
> -   them from getting globbed by the shell.  These two mean different
> -   things:
> +   them from getting globbed by the shell.  The following commands have
> +   two different meanings:
>  +
>  
>  $ git checkout -- *.c
> +
>  $ git checkout -- \*.c
> +$ git checkout -- "*.c"
> +$ git checkout -- '*.c'
>  
>  +
> -The former lets your shell expand the fileglob, and you are asking
> -the dot-C files in your working tree to be overwritten with the version
> -in the index.  The latter passes the `*.c` to Git, and you are asking
> -the paths in the index that match the pattern to be checked out to your
> -working tree.  After running `git add hello.c; rm hello.c`, you will _not_
> -see `hello.c` in your working tree with the former, but with the latter
> -you will.
> +The first command lets your shell expand the fileglob, and you are asking
> +the dot-C files in your working tree to be overwritten with the version in
> +the index.  The latter three variations pass the `*.c` to Git, and you are
> +asking the paths in the index that match the pattern to be checked out to
> +your working tree.  After running `git add hello.c; rm hello.c`, you will
> +_not_ see `hello.c` in your working tree with the first command, but with
> +the latter three variations, you will.
> 
>   * Just as the filesystem '.' (period) refers to the current directory,
> using a '.' as a repository name in Git (a dot-repository) is a relative
> 
> -- 
> 
> 
> Robert P. J. Day Ottawa, Ontario, CANADA
> http://crashcourse.ca
> 
> Twitter:   http://twitter.com/rpjday
> LinkedIn:   http://ca.linkedin.com/in/rpjday
> 

Re: [PATCH] doc: remove explanation of "--" from man pages

[Kevin Daudt]

On Tue, Nov 21, 2017 at 04:12:02PM -0500, Robert P. J. Day wrote:
> "man gitcli" already explains the purpose of the "--" syntax, so there
> is no value to a small subset of Git commands explaining that in their
> man pages.
> 
> Signed-off-by: Robert P. J. Day 
> 
> ---
> 
>   i tried this once before, and i'm going to try to push it through
> again ... it's pointless and inconsistent for less than a dozen man
> pages to explicitly explain the purpose of "--" unless all of the man
> pages do. as long as the "--" appears in the command SYNOPSIS, that
> should be more than adequate.

Although I agree that common options don't need to be explained
everytime again, this change might make '--' even more obscure. To be
honest, I didn't even know about gitcli(7), let alone most new users.

In the #git irc channel we often have to explain what '--' means and
why it's sometimes necessary.

I don't however know a better solution to it more clear.

Kevin

Re: Git on Mac - Segmentation fault:11

[Kevin Daudt]

On Wed, Nov 15, 2017 at 05:30:23PM -0700, Frank Burkitt wrote:
> I am using Git on a Macbook pro with MacOS High Sierra version 10.13.1
> (17B48).  I have been using it in a virtualenv with python 3.  I have
> begun to get "Segmentation fault: 11" with every git command.  I have
> been searching for a reason why this is occurring but have not been
> able to find a solution.  I have reinstalled the operating system,
> uninstalled and reinstalled Git, and a variety of other attempts at
> finding a solution.  Is this a know issue? Any suggestions would be
> appreciated.

Hello Frank,

Could you provide a bit more information?

- What version of git are you using?
- how did you install git,
- Do you also get segfaults outside of the virtualenv?

This sounds perhaps like it's a copattibility issue with a library.

Kind regards, Kevin.

Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kevin Daudt]

On Mon, Nov 13, 2017 at 08:01:12AM +0530, Kaartic Sivaraam wrote:
> On Sunday 12 November 2017 11:53 PM, Kevin Daudt wrote:
> > On Thu, Nov 02, 2017 at 12:24:07PM +0530, Kaartic Sivaraam wrote:
> > > From: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>
> > > 
> > > When trying to rename an inexistent branch to with a name of a branch
> > 
> > This sentence does not read well. Probably s/with a/the/ helps.
> > 
> 
> Thanks. Seems I missed it somehow. Will fix it.
> 
> > > that already exists the rename failed specifying the new branch name
> > > exists rather than specifying that the branch trying to be renamed
> > > doesn't exist.
> > > 
> > > [..]
> > > 
> > > Note: Thanks to the strbuf API that made it possible to easily construct
> > > the composite error message strings!
> > 
> > I'm not sure this note adds a lot, since the strbuf API is not that new.
> > 
> 
> That was a little attribution I wanted make to the strbuf API as this was
> the first time I leveraged it to this extent and I was surprised by the way
> it made string manipulation easier in C. Just documented my excitation. In
> case it seems to be noise (?) which should removed, let me know.

I guess that would fit better below the the ---

Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kevin Daudt]

On Thu, Nov 02, 2017 at 12:24:07PM +0530, Kaartic Sivaraam wrote:
> From: Kaartic Sivaraam 
> 
> When trying to rename an inexistent branch to with a name of a branch

This sentence does not read well. Probably s/with a/the/ helps.

> that already exists the rename failed specifying the new branch name
> exists rather than specifying that the branch trying to be renamed
> doesn't exist.
> 
> [..]
> 
> Note: Thanks to the strbuf API that made it possible to easily construct
> the composite error message strings!

I'm not sure this note adds a lot, since the strbuf API is not that new.

Kevin

Re: [PATCH] config: added --expiry-date type support

[Kevin Daudt]

On Sun, Nov 12, 2017 at 12:19:35PM +, Haaris wrote:
> ---
>  builtin/config.c   | 11 ++-
>  config.c   |  9 +
>  config.h   |  1 +
>  t/t1300-repo-config.sh | 25 +
>  4 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/config.c b/builtin/config.c
> index d13daeeb55927..41cd9f5ca7cde 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -52,6 +52,7 @@ static int show_origin;
>  #define TYPE_INT (1<<1)
>  #define TYPE_BOOL_OR_INT (1<<2)
>  #define TYPE_PATH (1<<3)
> +#define TYPE_EXPIRY_DATE (1<<4)
>  
>  static struct option builtin_config_options[] = {
>   OPT_GROUP(N_("Config file location")),
> @@ -80,6 +81,7 @@ static struct option builtin_config_options[] = {
>   OPT_BIT(0, "int", , N_("value is decimal number"), TYPE_INT),
>   OPT_BIT(0, "bool-or-int", , N_("value is --bool or --int"), 
> TYPE_BOOL_OR_INT),
>   OPT_BIT(0, "path", , N_("value is a path (file or directory 
> name)"), TYPE_PATH),
> + OPT_BIT(0, "expiry-date", , N_("value is an expiry date"), 
> TYPE_EXPIRY_DATE),
>   OPT_GROUP(N_("Other")),
>   OPT_BOOL('z', "null", _null, N_("terminate values with NUL byte")),
>   OPT_BOOL(0, "name-only", _values, N_("show variable names only")),
> @@ -159,6 +161,12 @@ static int format_config(struct strbuf *buf, const char 
> *key_, const char *value
>   return -1;
>   strbuf_addstr(buf, v);
>   free((char *)v);
> + } else if (types == TYPE_EXPIRY_DATE) {
> + timestamp_t *t = malloc(sizeof(*t));
> + if(git_config_expiry_date(, key_, value_) < 0)
> + return -1;
> + strbuf_addf(buf, "%"PRItime, *t);
> + free((timestamp_t *)t);
>   } else if (value_) {
>   strbuf_addstr(buf, value_);
>   } else {
> @@ -273,12 +281,13 @@ static char *normalize_value(const char *key, const 
> char *value)
>   if (!value)
>   return NULL;
>  
> - if (types == 0 || types == TYPE_PATH)
> + if (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)
>   /*
>* We don't do normalization for TYPE_PATH here: If
>* the path is like ~/foobar/, we prefer to store
>* "~/foobar/" in the config file, and to expand the ~
>* when retrieving the value.
> +  * Also don't do normalization for expiry dates.
>*/
>   return xstrdup(value);
>   if (types == TYPE_INT)
> diff --git a/config.c b/config.c
> index 903abf9533b18..caa2fd5fb6915 100644
> --- a/config.c
> +++ b/config.c
> @@ -990,6 +990,15 @@ int git_config_pathname(const char **dest, const char 
> *var, const char *value)
>   return 0;
>  }
>  
> +int git_config_expiry_date(timestamp_t **timestamp, const char *var, const 
> char *value)
> +{
> + if (!value)
> + return config_error_nonbool(var);
> + if (!!parse_expiry_date(value, *timestamp))
> + die(_("failed to parse date_string in: '%s'"), value);
> + return 0;
> +}
> +
>  static int git_default_core_config(const char *var, const char *value)
>  {
>   /* This needs a better name */
> diff --git a/config.h b/config.h
> index a49d264416225..2d127d9d23c90 100644
> --- a/config.h
> +++ b/config.h
> @@ -58,6 +58,7 @@ extern int git_config_bool_or_int(const char *, const char 
> *, int *);
>  extern int git_config_bool(const char *, const char *);
>  extern int git_config_string(const char **, const char *, const char *);
>  extern int git_config_pathname(const char **, const char *, const char *);
> +extern int git_config_expiry_date(timestamp_t **, const char *, const char 
> *);
>  extern int git_config_set_in_file_gently(const char *, const char *, const 
> char *);
>  extern void git_config_set_in_file(const char *, const char *, const char *);
>  extern int git_config_set_gently(const char *, const char *);
> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 364a537000bbb..59a35be89e511 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -901,6 +901,31 @@ test_expect_success 'get --path barfs on boolean 
> variable' '
>   test_must_fail git config --get --path path.bool
>  '
>  
> +test_expect_success 'get --expiry-date' '
> + cat >.git/config <<-\EOF &&
> + [date]
> + valid1 = "Fri Jun 4 15:46:55 2010"
> + valid2 = "2017/11/11 11:11:11PM"
> + valid3 = "2017/11/10 09:08:07 PM"
> + valid4 = "never"
> + invalid1 = "abc"
> + EOF
> + cat >expect <<-\EOF &&
> + 1275666415
> + 1510441871
> + 1510348087
> + 0
> + EOF
> + {
> + git config --expiry-date date.valid1 &&
> + git config --expiry-date date.valid2 &&
> + git config --expiry-date date.valid3 &&

Re: [PATCH] bisect: clarify that one can select multiple good commits

[Kevin Daudt]

On Sat, Nov 11, 2017 at 08:26:00AM -0500, Robert P. J. Day wrote:
> 
> Current man page for "bisect" is inconsistent explaining the fact that
> "git bisect" takes precisely one bad commit, but one or more good
> commits, so tweak the man page in a few places to make that clear.
> 
> rday
> 
> Signed-off-by: Robert P. J. Day 
> 
> ---
> 
>   i also exercised literary license to reword an example to look for a
> commit where performance was *degraded* rather than improved, since i
> think that's the sort of thing that people would be more interested
> in.
> 
>  In fact, `git bisect` can be used to find the commit that changed
>  *any* property of your project; e.g., the commit that fixed a bug, or
> -the commit that caused a benchmark's performance to improve. To
> +the commit that caused a benchmark's performance to degrade. To
>  support this more general usage, the terms "old" and "new" can be used
>  in place of "good" and "bad", or you can choose your own terms. See
>  section "Alternate terms" below for more information.
> @@ -58,7 +58,7 @@ $ git bisect bad # Current version is bad
>  $ git bisect good v2.6.13-rc2# v2.6.13-rc2 is known to be good
>  
> 

I think this example was meant to suggest that it's not only possible to
find bad things (bugs, performance degradations), but also the opposite
(when was a bug fixed, what caused the performance to change). 

So I think it's good to keep the example like it is.

Re: [PATCH 1/3] notes: move hex_to_bytes() to hex.c and export it

[Kevin Daudt]

On Sun, Nov 05, 2017 at 05:47:47PM +0100, René Scharfe wrote:
> Am 05.11.2017 um 03:56 schrieb Kevin Daudt:
> > On Tue, Oct 31, 2017 at 02:46:49PM +0100, René Scharfe wrote:
> >> Make the function for converting pairs of hexadecimal digits to binary
> >> available to other call sites.
> >>
> >> Signed-off-by: Rene Scharfe <l@web.de>
> >> ---
> >>   cache.h |  7 +++
> >>   hex.c   | 12 
> >>   notes.c | 17 -
> >>   3 files changed, 19 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/cache.h b/cache.h
> >> index 6440e2bf21..f06bfbaf32 100644
> >> --- a/cache.h
> >> +++ b/cache.h
> >> @@ -1317,6 +1317,13 @@ extern int set_disambiguate_hint_config(const char 
> >> *var, const char *value);
> >>   extern int get_sha1_hex(const char *hex, unsigned char *sha1);
> >>   extern int get_oid_hex(const char *hex, struct object_id *sha1);
> >>   
> >> +/*
> >> + * Read `len` pairs of hexadecimal digits from `hex` and write the
> >> + * values to `binary` as `len` bytes. Return 0 on success, or -1 if
> > 
> > Is it correct to call the result binary? I would say that it's the value
> > that gets stored. To me, this value does not really have a base.
> 
> Here's the full context:
> 
>   /*
>* Read `len` pairs of hexadecimal digits from `hex` and write the
>* values to `binary` as `len` bytes. Return 0 on success, or -1 if
>* the input does not consist of hex digits).
>*/
>   extern int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
> 
> The patch moves the comment verbatim.  Words in backticks (`binary`,
> `hex`, `len`) are parameter names.
> 
> The function converts pairs of hexadecimal digits (base 16, ASCII
> encoded) to bytes (base 256).  A byte can be seen as an array of bits;
> thus the output is also binary (base 2) without requiring further
> conversion.
> 
> Calling the variable "binary" may seem unspecific, but makes sense in
> the context of this function.
> 
> Does any of that help?
> 
> Thanks,
> René

Thanks, I have been thinking about it more, and I agree, it does make
sense. 

I had a binary representation in mind, but this is refering to binary
data (just like you can have binary files).

Kevin

Re: [PATCH 1/3] notes: move hex_to_bytes() to hex.c and export it

[Kevin Daudt]

On Tue, Oct 31, 2017 at 02:46:49PM +0100, René Scharfe wrote:
> Make the function for converting pairs of hexadecimal digits to binary
> available to other call sites.
> 
> Signed-off-by: Rene Scharfe 
> ---
>  cache.h |  7 +++
>  hex.c   | 12 
>  notes.c | 17 -
>  3 files changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/cache.h b/cache.h
> index 6440e2bf21..f06bfbaf32 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1317,6 +1317,13 @@ extern int set_disambiguate_hint_config(const char 
> *var, const char *value);
>  extern int get_sha1_hex(const char *hex, unsigned char *sha1);
>  extern int get_oid_hex(const char *hex, struct object_id *sha1);
>  
> +/*
> + * Read `len` pairs of hexadecimal digits from `hex` and write the
> + * values to `binary` as `len` bytes. Return 0 on success, or -1 if

Is it correct to call the result binary? I would say that it's the value
that gets stored. To me, this value does not really have a base.

Kevin

Re: [PATCH v1 1/2] refs: extract function to normalize partial refs

[Kevin Daudt]

On Sat, Nov 04, 2017 at 11:27:39AM +0900, Junio C Hamano wrote:
> I however notice that addition of /* to the tail is trying to be
> careful by using strbuf_complete('/'), but prefixing with "refs/"
> does not and we would end up with a double-slash if pattern begins
> with a slash.  The contract between the caller of this function (or
> its original, which is for_each_glob_ref_in()) and the callee is
> that prefix must not begin with '/', so it may be OK, but we might
> want to add "if (*pattern == '/') BUG(...)" at the beginning.
>
> I dunno.  In any case, that is totally outside the scope of this two
> patch series.

I do think it's a good idea to make future readers of the code aware of
this contract, and adding a BUG assert does that quite well. Here is a
patch that implements it.

This applies of course on top of this patch series.

-- >8 --
Subject: [PATCH] normalize_glob_ref: assert implicit contract of prefix

normalize_glob_ref has an implicit contract of expecting 'prefix' to not
start with a '/', otherwise the pattern would end up with a
double-slash.

Mark it as a BUG when the prefix argument of normalize_glob_ref starts
with a '/' so that future callers will be aware of this contract.

Signed-off-by: Kevin Daudt <m...@ikke.info>
---
 refs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/refs.c b/refs.c
index e9ae659ae..6747981d1 100644
--- a/refs.c
+++ b/refs.c
@@ -372,6 +372,8 @@ int head_ref_namespaced(each_ref_fn fn, void *cb_data)
 void normalize_glob_ref(struct strbuf *normalized_pattern, const char *prefix,
const char *pattern, int flags)
 {
+   if (prefix && *prefix == '/') BUG("prefix cannot not start with '/'");
+
if (!prefix && !starts_with(pattern, "refs/"))
strbuf_addstr(normalized_pattern, "refs/");
else if (prefix)
-- 
2.15.0.rc2.57.g2f899857a9

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Kevin Daudt]

On Tue, Oct 31, 2017 at 05:33:57PM +0100, Kevin Daudt wrote:
> On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote:
> > Reduce code duplication by extracting a function for rewriting an
> > existing file.
> > 
> > Signed-off-by: Rene Scharfe <l@web.de>
> > ---
> >  sequencer.c | 46 +-
> >  1 file changed, 17 insertions(+), 29 deletions(-)
> > 
> > diff --git a/sequencer.c b/sequencer.c > > index f2a10cc4f2..17360eb38a 
> > 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2665,6 +2665,20 @@ int check_todo_list(void)
> > return res;
> >  }
> >  
> > +static int rewrite_file(const char *path, const char *buf, size_t len)
> > +{
> > +   int rc = 0;
> > +   int fd = open(path, O_WRONLY);
> > +   if (fd < 0)
> > +   return error_errno(_("could not open '%s' for writing"), path);
> > +   if (write_in_full(fd, buf, len) < 0)
> > +   rc = error_errno(_("could not write to '%s'"), path);
> > +   if (!rc && ftruncate(fd, len) < 0)
> > +   rc = error_errno(_("could not truncate '%s'"), path);
> > +   close(fd);
> > +   return rc;
> > +}
> > +
> >  /* skip picking commits whose parents are unchanged */
> >  int skip_unnecessary_picks(void)
> >  {
> > @@ -2737,29 +2751,11 @@ int skip_unnecessary_picks(void)
> > }
> > close(fd);
> >  
> > -   fd = open(rebase_path_todo(), O_WRONLY, 0666);
> > -   if (fd < 0) {
> > -   error_errno(_("could not open '%s' for writing"),
> > -   rebase_path_todo());
> > +   if (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset,
> > +todo_list.buf.len - offset) < 0) {
> > todo_list_release(_list);
> > return -1;
> > }
> > -   if (write_in_full(fd, todo_list.buf.buf + offset,
> > -   todo_list.buf.len - offset) < 0) {
> > -   error_errno(_("could not write to '%s'"),
> > -   rebase_path_todo());
> > -   close(fd);
> > -   todo_list_release(_list);
> 
> Is this missing on purpose in the new situation?
>

I wasn't looking at the context, only the changed lines. After reading
it again, it's clear that nothing is missing (the freeing of todo_list).

Kevin

Re: Is it possible to convert a Json file to xml file with Git

[Kevin Daudt]

On Tue, Oct 31, 2017 at 05:28:40PM +, Eyjolfur Eyjolfsson wrote:
> Hi
> 
> I have a question.
> Is it possible to convert a Json file to XML with Git
> 
> Best regards
> 
> Eyjolfur Eyjolfsson
> 
> (e) eyjolfureyjolfs...@tprg.com
> (w) tpretailgroup.com
> 

Hello Eyjolfur,

git is a version control system, which is mostly content agnostic. It
knows nothing about json or xml, let alone how to convert them.

You might want to use some kind of programming language to do the
conversion.

Could you perhaps explain more why you are asking this question?

Kevin.

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Kevin Daudt]

On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote:
> Reduce code duplication by extracting a function for rewriting an
> existing file.
> 
> Signed-off-by: Rene Scharfe 
> ---
>  sequencer.c | 46 +-
>  1 file changed, 17 insertions(+), 29 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index f2a10cc4f2..17360eb38a 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2665,6 +2665,20 @@ int check_todo_list(void)
>   return res;
>  }
>  
> +static int rewrite_file(const char *path, const char *buf, size_t len)
> +{
> + int rc = 0;
> + int fd = open(path, O_WRONLY);
> + if (fd < 0)
> + return error_errno(_("could not open '%s' for writing"), path);
> + if (write_in_full(fd, buf, len) < 0)
> + rc = error_errno(_("could not write to '%s'"), path);
> + if (!rc && ftruncate(fd, len) < 0)
> + rc = error_errno(_("could not truncate '%s'"), path);
> + close(fd);
> + return rc;
> +}
> +
>  /* skip picking commits whose parents are unchanged */
>  int skip_unnecessary_picks(void)
>  {
> @@ -2737,29 +2751,11 @@ int skip_unnecessary_picks(void)
>   }
>   close(fd);
>  
> - fd = open(rebase_path_todo(), O_WRONLY, 0666);
> - if (fd < 0) {
> - error_errno(_("could not open '%s' for writing"),
> - rebase_path_todo());
> + if (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset,
> +  todo_list.buf.len - offset) < 0) {
>   todo_list_release(_list);
>   return -1;
>   }
> - if (write_in_full(fd, todo_list.buf.buf + offset,
> - todo_list.buf.len - offset) < 0) {
> - error_errno(_("could not write to '%s'"),
> - rebase_path_todo());
> - close(fd);
> - todo_list_release(_list);

Is this missing on purpose in the new situation?

> - return -1;
> - }
> - if (ftruncate(fd, todo_list.buf.len - offset) < 0) {
> - error_errno(_("could not truncate '%s'"),
> - rebase_path_todo());
> - todo_list_release(_list);
> - close(fd);
> - return -1;
> - }
> - close(fd);
>  
>   todo_list.current = i;
>   if (is_fixup(peek_command(_list, 0)))
> @@ -2944,15 +2940,7 @@ int rearrange_squash(void)
>   }
>   }
>  
> - fd = open(todo_file, O_WRONLY);
> - if (fd < 0)
> - res = error_errno(_("could not open '%s'"), todo_file);
> - else if (write(fd, buf.buf, buf.len) < 0)
> - res = error_errno(_("could not write to '%s'"), 
> todo_file);
> - else if (ftruncate(fd, buf.len) < 0)
> - res = error_errno(_("could not truncate '%s'"),
> -todo_file);
> - close(fd);
> + res = rewrite_file(todo_file, buf.buf, buf.len);
>   strbuf_release();
>   }
>  
> -- 
> 2.15.0

Except for that question, it looks good to me (as a beginner), it makes
the code better readable.

Re: [PATCH 2/2] sequencer: use O_TRUNC to truncate files

[Kevin Daudt]

On Tue, Oct 31, 2017 at 10:58:16AM +0100, René Scharfe wrote:
> Cut off any previous content of the file to be rewritten by passing the
> flag O_TRUNC to open(2) instead of calling ftruncate(2) at the end.
> That's easier and shorter.
> 
> Signed-off-by: Rene Scharfe 
> ---
>  sequencer.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 17360eb38a..f93b60f615 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2668,13 +2668,11 @@ int check_todo_list(void)
>  static int rewrite_file(const char *path, const char *buf, size_t len)
>  {
>   int rc = 0;
> - int fd = open(path, O_WRONLY);
> + int fd = open(path, O_WRONLY | O_TRUNC);
>   if (fd < 0)
>   return error_errno(_("could not open '%s' for writing"), path);
>   if (write_in_full(fd, buf, len) < 0)
>   rc = error_errno(_("could not write to '%s'"), path);
> - if (!rc && ftruncate(fd, len) < 0)
> - rc = error_errno(_("could not truncate '%s'"), path);
>   close(fd);
>   return rc;
>  }
> -- 
> 2.15.0

Makes sense

Re: [PATCH 3/3] builtin/describe: describe blobs

[Kevin Daudt]

On Sat, Oct 28, 2017 at 08:28:54PM -0700, Stefan Beller wrote:
> On Sat, Oct 28, 2017 at 10:32 AM, Johannes Schindelin
>  wrote:
> > 
> > [..]
> > I wonder whether it would make sense to extend this to tree objects while
> > we are at it, but maybe that's an easy up-for-grabs.
> 
> I can look into incorporating that, too. What is the use case though?
> (Is there any error message, common enough that users want to
> identify trees?)
> 
> Thanks for the review,
> Stefan

Not sure if it's really helpfulp, but sometimes with corrup repos, git
would complain about a certain object missing or corrupt, where it might
be usefull to find out how it's referenced. Not sure though if this
would work in that case, because the repo is already corrupt.

Kevin

Re: [PATCH] cherry-pick: add --keep-existing-origin option

[Kevin Daudt]

On Sat, Oct 28, 2017 at 08:04:40PM +0800, Anthony Wong wrote:
> When cherry-picking from a commit whose commit message already
> contains the "(cherry picked from commit ...)" line, this option will
> not add another one. This is useful when you are cherry-picking from a
> bunch of commits, some are cherry-picks and already contains the
> upstream hash but some do not. Use with -x.
> 
> Signed-off-by: Anthony Wong 
> ---
>  Documentation/git-cherry-pick.txt |  8 
>  builtin/revert.c  |  2 ++
>  sequencer.c   | 14 --
>  sequencer.h   |  1 +
>  4 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/git-cherry-pick.txt 
> b/Documentation/git-cherry-pick.txt
> index d35d771fc..7a074511f 100644
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -71,6 +71,14 @@ OPTIONS
>   development branch), adding this information can be
>   useful.
>  
> +--keep-existing-origin::
> + This option has to be used with -x to take effect. When
> + cherry-picking from a commit whose commit message already
> + contains the "(cherry picked from commit ...)" line, this
> + option will not add another one. This is useful when you are
> + cherry-picking from a bunch of commits, some are cherry-picks
> + and already contains the upstream hash but some do not.
> +
>  -r::
>   It used to be that the command defaulted to do `-x`
>   described above, and `-r` was to disable it.  Now the
> diff --git a/builtin/revert.c b/builtin/revert.c
> index b9d927eb0..a1900cc1d 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -122,6 +122,7 @@ static int run_sequencer(int argc, const char **argv, 
> struct replay_opts *opts)
>   OPT_BOOL(0, "allow-empty", >allow_empty, 
> N_("preserve initially empty commits")),
>   OPT_BOOL(0, "allow-empty-message", 
> >allow_empty_message, N_("allow commits with empty messages")),
>   OPT_BOOL(0, "keep-redundant-commits", 
> >keep_redundant_commits, N_("keep redundant, empty commits")),
> + OPT_BOOL(0, "keep-existing-origin", 
> >keep_existing_origin, N_("do not add another hash if one already 
> exists, use with -x")),
>   OPT_END(),
>   };
>   options = parse_options_concat(options, cp_extra);
> @@ -157,6 +158,7 @@ static int run_sequencer(int argc, const char **argv, 
> struct replay_opts *opts)
>   "--ff", opts->allow_ff,
>   "--rerere-autoupdate", opts->allow_rerere_auto 
> == RERERE_AUTOUPDATE,
>   "--no-rerere-autoupdate", 
> opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,
> + "--keep-existing-origin", 
> opts->keep_existing_origin,
>   NULL);
>   }
>  
> diff --git a/sequencer.c b/sequencer.c
> index f2a10cc4f..c96add16e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1050,12 +1050,14 @@ static int do_pick_commit(enum todo_command command, 
> struct commit *commit,
>   strbuf_addstr(, p);
>  
>   if (opts->record_origin) {
> - strbuf_complete_line();
> - if (!has_conforming_footer(, NULL, 0))
> - strbuf_addch(, '\n');
> - strbuf_addstr(, cherry_picked_prefix);
> - strbuf_addstr(, oid_to_hex(>object.oid));
> - strbuf_addstr(, ")\n");
> + if (!opts->keep_existing_origin || strstr(msgbuf.buf, 
> cherry_picked_prefix) == NULL) {
> + strbuf_complete_line();
> + if (!has_conforming_footer(, NULL, 0))
> + strbuf_addch(, '\n');
> + strbuf_addstr(, cherry_picked_prefix);
> + strbuf_addstr(, 
> oid_to_hex(>object.oid));
> + strbuf_addstr(, ")\n");
> + }
>   }
>   }
>  
> diff --git a/sequencer.h b/sequencer.h
> index 6f3d3df82..a907c0947 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -24,6 +24,7 @@ struct replay_opts {
>   int allow_empty;
>   int allow_empty_message;
>   int keep_redundant_commits;
> + int keep_existing_origin;
>   int verbose;
>  
>   int mainline;
> -- 
> 2.14.1
> 

I'm wondering if it isn't better to detect that there is already an
origin present and not add another one. 

Or are there situations where you do want multiple cherry-pick origins?

Kevin

Re: [PATCH] status: do not get confused by submodules in excluded directories

[Kevin Daudt]

On Tue, Oct 17, 2017 at 03:10:11PM +0200, Johannes Schindelin wrote:
> We meticulously pass the `exclude` flag to the `treat_directory()`
> function so that we can indicate that files in it are excluded rather
> than untracked when recursing.
> 
> But we did not yet treat submodules the same way.
> 
> Signed-off-by: Johannes Schindelin 
> ---
> Published-As: 
> https://github.com/dscho/git/releases/tag/submodule-in-excluded-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git submodule-in-excluded-v1
>  dir.c  |  2 +-
>  t/t7061-wtstatus-ignore.sh | 14 ++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/dir.c b/dir.c
> index 1d17b800cf3..9987011da57 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1392,7 +1392,7 @@ static enum path_treatment treat_directory(struct 
> dir_struct *dir,
>   if (!(dir->flags & DIR_NO_GITLINKS)) {
>   unsigned char sha1[20];
>   if (resolve_gitlink_ref(dirname, "HEAD", sha1) == 0)
> - return path_untracked;
> + return exclude ? path_excluded : path_untracked;
>   }
>   return path_recurse;
>   }
> diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
> index fc6013ba3c8..8c849a4cd2f 100755
> --- a/t/t7061-wtstatus-ignore.sh
> +++ b/t/t7061-wtstatus-ignore.sh
> @@ -272,4 +272,18 @@ test_expect_success 'status ignored tracked directory 
> with uncommitted file in t
>   test_cmp expected actual
>  '
>  
> +cat >expected <<\EOF
> +!! tracked/submodule/
> +EOF
> +
> +test_expect_success 'status ignores submodule in excluded directory' '
> + git init tracked/submodule &&
> + (
> + cd tracked/submodule &&
> + test_commit initial
> + ) &&

Could this use test_commit -C tracked/submodule initial?

> + git status --porcelain --ignored -u tracked/submodule >actual &&
> + test_cmp expected actual
> +'
> +
>  test_done
> 
> base-commit: 111ef79afe185f8731920569450f6a65320f5d5f
> -- 
> 2.14.2.windows.3

Re: [PATCH v2] column: show auto columns when pager is active

[Kevin Daudt]

On Mon, Oct 23, 2017 at 02:52:46PM -0700, Jonathan Nieder wrote:
> Hi,
> 
> Kevin Daudt wrote:
> 
> > --- a/column.c
> > +++ b/column.c
> > @@ -5,6 +5,7 @@
> >  #include "parse-options.h"
> >  #include "run-command.h"
> >  #include "utf8.h"
> > +#include "pager.c"
> 
> Should this be pager.h?
> 
> Thanks,
> Jonathan

Thanks for catching this.

Re: [PATCH v2 1/9] perf/run: add '--config' option to the 'run' script

[Kevin Daudt]

On Sat, Sep 23, 2017 at 07:55:56PM +, Christian Couder wrote:
> It is error prone and tiring to use many long environment
> variables to give parameters to the 'run' script.
> 
> Let's make it easy to store some parameters in a config
> file and to pass them to the run script.
> 
> The GIT_PERF_CONFIG_FILE variable will be set to the
> argument of the '--config' option. This variable is not
> used yet. It will be used in a following commit.
> 
> Signed-off-by: Christian Couder 
> ---
>  t/perf/run | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/t/perf/run b/t/perf/run
> index beb4acc0e428d..1e7c2a59e45dc 100755
> --- a/t/perf/run
> +++ b/t/perf/run
> @@ -2,9 +2,14 @@
>  
>  case "$1" in
>   --help)
> - echo "usage: $0 [other_git_tree...] [--] [test_scripts]"
> + echo "usage: $0 [--config file] [other_git_tree...] [--] 
> [test_scripts]"
>   exit 0
>   ;;
> + --config)
> + shift
> + GIT_PERF_CONFIG_FILE=$(cd "$(dirname "$1")"; pwd)/$(basename 
> "$1")

Is the idea of this construct to do some kind of normalization?
Otherwise it seems to just result in $1 again.

> + export GIT_PERF_CONFIG_FILE
> + shift ;;
>  esac
>  
>  die () {
> 
> --
> https://github.com/git/git/pull/408

Re: Multiple paths in GIT_EXEC_PATH

[Kevin Daudt]

On Tue, Oct 17, 2017 at 06:21:22PM +0300, Nikolay Yakimov wrote:
> Hello. After updating to a recent git release (2.14, I believe, but
> possibly earlier), setting GIT_EXEC_PATH to multiple directories
> stopped working. It did work before, and I believe the culprit is
> 'git-sh-setup', which uses 'git --exec-path' output directly, while
> most other git components observe PATH syntax, i.e. multiple paths
> with colon separator. Is this intended, or is it an oversight?
> 
> For why I need that is another matter. Long story short, I need git to
> look for '.gitignore' in a particular non-standard location, since I
> have multiple git repositories in the same workdir (that workdir being
> $HOME and git repositories being stores for my different configs)

The commit that changed what you described is 1073094f3 (git-sh-setup:
be explicit where to dot-source git-sh-i18n from., 2016-10-29). That
commit claims there were already scripts that assumed GIT_EXEC_PATH is
just a single entry. That commit was included in v2.11.

There was also a recent thread[0] about it that discussed this issue,
where someone stated that indeed treating GIT_EXEC_PATH with the same
semantics as PATH has been broken for a while, but it seems there are no
real plans to fix it.

[0]:https://public-inbox.org/git/20170928223134.GA30744@varnish/

Re: [PATCH] check-ref-format: require a repository for --branch

[Kevin Daudt]

On Tue, Oct 17, 2017 at 11:40:17AM +0900, Junio C Hamano wrote:
> Kevin Daudt <m...@ikke.info> writes:
> 
> >> +  setup_git_directory_gently();
> >> +
> >> +  if (!nongit)
> >> +  malformed = (strbuf_check_branch_ref(, arg) ||
> >> +   !skip_prefix(sb.buf, "refs/heads/", ));
> >> +  else
> >> +  malformed = check_branch_ref_format(arg);
> >> +
> >
> > Would it make sense to swap the logic and get rid of the double
> > negative (!nongit)?
> 
> I am trying to follow the pattern "handle the normal case that have
> been supported forever first, and then handle new exception next",
> so that it is easier to see that there is no behaviour change in the
> normal case, so I do not think it makes it easier to see to swap the
> if/else cases.

Ok, thanks for your reasoning, makes sense.
> >
> >> +  if (malformed)
> >>die("'%s' is not a valid branch name", arg);
> >> -  printf("%s\n", sb.buf + 11);
> >> +  printf("%s\n", name);
> >> +  strbuf_release();
> >>return 0;
> >>  }
> >>  

Re: [PATCH] check-ref-format: require a repository for --branch

[Kevin Daudt]

On Mon, Oct 16, 2017 at 07:45:46PM +0900, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
> [..]
> 
> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
> index 1e5f9835f0..4e62852089 100644
> --- a/builtin/check-ref-format.c
> +++ b/builtin/check-ref-format.c
> @@ -38,12 +38,22 @@ static char *collapse_slashes(const char *refname)
>  
>  static int check_ref_format_branch(const char *arg)
>  {
> + int nongit, malformed;
>   struct strbuf sb = STRBUF_INIT;
> + const char *name = arg;
>  
> - setup_git_directory();
> - if (strbuf_check_branch_ref(, arg))
> + setup_git_directory_gently();
> +
> + if (!nongit)
> + malformed = (strbuf_check_branch_ref(, arg) ||
> +  !skip_prefix(sb.buf, "refs/heads/", ));
> + else
> + malformed = check_branch_ref_format(arg);
> +

Would it make sense to swap the logic and get rid of the double
negative (!nongit)?

> + if (malformed)
>   die("'%s' is not a valid branch name", arg);
> - printf("%s\n", sb.buf + 11);
> + printf("%s\n", name);
> + strbuf_release();
>   return 0;
>  }
>  

Re: [PATCH v3 00/25] object_id part 10

[Kevin Daudt]

On Mon, Oct 16, 2017 at 11:49:13PM +, brian m. carlson wrote:
> On Mon, Oct 16, 2017 at 11:15:34AM +0900, Junio C Hamano wrote:
> > With a hope that this might help other reviewers, here is the
> > interdiff between "the previous one merged with v2.15-rc1" and "this
> > round applied on v2.15-rc1 directly".  
> > 
> > The changes all looked sensible to me.  Thanks.
> 
> Is there a reasonably straightforward tool or workflow to generate
> interdiffs?  If so, I can include them in the future.
> -- 
> brian m. carlson / brian with sandals: Houston, Texas, US
> https://www.crustytoothpaste.net/~bmc | My opinion only
> OpenPGP: https://keybase.io/bk2204

tbdiff: https://github.com/trast/tbdiff

Re: [PATCH v4 03/11] protocol: introduce protocol extension mechanisms

[Kevin Daudt]

On Mon, Oct 16, 2017 at 10:55:24AM -0700, Brandon Williams wrote:
> Create protocol.{c,h} and provide functions which future servers and
> clients can use to determine which protocol to use or is being used.
> 
> Also introduce the 'GIT_PROTOCOL' environment variable which will be
> used to communicate a colon separated list of keys with optional values
> to a server.  Unknown keys and values must be tolerated.  This mechanism
> is used to communicate which version of the wire protocol a client would
> like to use with a server.
> 
> Signed-off-by: Brandon Williams 
> ---
>  Documentation/config.txt | 17 +++
>  Documentation/git.txt|  6 
>  Makefile |  1 +
>  cache.h  |  8 +
>  protocol.c   | 79 
> 
>  protocol.h   | 33 
>  6 files changed, 144 insertions(+)
>  create mode 100644 protocol.c
>  create mode 100644 protocol.h
>
> [...]
> 
> diff --git a/protocol.h b/protocol.h
> new file mode 100644
> index 0..1b2bc94a8
> --- /dev/null
> +++ b/protocol.h
> @@ -0,0 +1,33 @@
> +#ifndef PROTOCOL_H
> +#define PROTOCOL_H
> +
> +enum protocol_version {
> + protocol_unknown_version = -1,
> + protocol_v0 = 0,
> + protocol_v1 = 1,
> +};
> +
> +/*
> + * Used by a client to determine which protocol version to request be used 
> when
> + * communicating with a server, reflecting the configured value of the
> + * 'protocol.version' config.  If unconfigured, a value of 'protocol_v0' is
> + * returned.
> + */

The first sentence reads a little weird to me around 'which version to
request be used'. 

[PATCH v2] column: show auto columns when pager is active

[Kevin Daudt]

When columns are set to automatic for git tag and the output is
paginated by git, the output is a single column instead of multiple
columns.

Standard behaviour in git is to honor auto values when the pager is
active, which happens for example with commands like git log showing
colors when being paged.

Since ff1e72483 (tag: change default of `pager.tag` to "on",
2017-08-02), the pager has been enabled by default, exposing this
problem to more people.

finalize_colopts in column.c only checks whether the output is a TTY to
determine if columns should be enabled with columns set to auto. Also
check if the pager is active.

Adding a test for git column is possible but requires some care to work
around a race on stdin. See commit 18d8c2693 (test_terminal: redirect
child process' stdin to a pty, 2015-08-04). Test git tag instead, since
that does not involve stdin, and since that was the original motivation
for this patch.

Helped-by: Rafael Ascensão <rafa.al...@gmail.com>
Signed-off-by: Kevin Daudt <m...@ikke.info>
---
Changes since v1:

- Remove redundant -p flag in tests
- Explain why git tag is being tested instead of the more obvious git
  column

 column.c |  3 ++-
 t/t7006-pager.sh | 14 ++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/column.c b/column.c
index ff7bdab1a..ded50337f 100644
--- a/column.c
+++ b/column.c
@@ -5,6 +5,7 @@
 #include "parse-options.h"
 #include "run-command.h"
 #include "utf8.h"
+#include "pager.c"
 
 #define XY2LINEAR(d, x, y) (COL_LAYOUT((d)->colopts) == COL_COLUMN ? \
(x) * (d)->rows + (y) : \
@@ -224,7 +225,7 @@ int finalize_colopts(unsigned int *colopts, int 
stdout_is_tty)
if (stdout_is_tty < 0)
stdout_is_tty = isatty(1);
*colopts &= ~COL_ENABLE_MASK;
-   if (stdout_is_tty)
+   if (stdout_is_tty || pager_in_use())
*colopts |= COL_ENABLED;
}
return 0;
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index f0f1abd1c..e985b6873 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -570,4 +570,18 @@ test_expect_success 'command with underscores does not 
complain' '
test_cmp expect actual
 '
 
+test_expect_success TTY 'git tag with auto-columns ' '
+   test_commit one &&
+   test_commit two &&
+   test_commit three &&
+   test_commit four &&
+   test_commit five &&
+   cat >expected <<\EOF &&
+initial  one  two  threefour five
+EOF
+   test_terminal env PAGER="cat >actual.tag" COLUMNS=80 \
+   git -c column.ui=auto tag --sort=authordate &&
+   test_cmp expected actual.tag
+'
+
 test_done
-- 
2.14.2

Re: Can I remove multiple stashed states at a time?

[Kevin Daudt]

On Fri, Oct 13, 2017 at 01:35:22PM -0400, Jeff King wrote:
> On Fri, Oct 13, 2017 at 11:58:12AM +0900, 小川恭史 wrote:
> 
> You can also just do:
> 
>   for i in 1 2 3; do
>  git stash drop $i
>   done
> 

Doesn't stash 2 become stash 1 after the first drop, and the same for 3,
resulting in dropping stash 1, 3 and 5?

So something like

  for i in 3 2 1; do
  git stash drop $i;
  done

Or leave $i out altogether.

Re: [PATCH v2 2/3] for-each-ref: let upstream/push optionally remote ref name

[Kevin Daudt]

On Thu, Oct 05, 2017 at 02:19:15PM +0200, Johannes Schindelin wrote:
> From: J Wyman 
> [..] 
> 
> diff --git a/Documentation/git-for-each-ref.txt 
> b/Documentation/git-for-each-ref.txt
> index 39f50bd53eb..22767025850 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -142,8 +142,9 @@ upstream::
>   encountered. Append `:track,nobracket` to show tracking
>   information without brackets (i.e "ahead N, behind M").
>  +
> -Also respects `:remotename` to state the name of the *remote* instead of
> -the ref.
> +Also respects `:remotename` to state the name of the *remote* instead
> +of the ref, and `:remoteref` to state the name of the *reference* as
> +locally known by the remote.

What does "locally known by the remote" mean in this sentence?

>  +
>  Has no effect if the ref does not have tracking information associated
>  with it.  All the options apart from `nobracket` are mutually exclusive,
> @@ -152,9 +153,9 @@ but if used together the last option is selected.

Re: [PATCH] Documentation/merge-options.txt: Add -S/--gpg-sign

[Kevin Daudt]

On Thu, Oct 12, 2017 at 12:44:59PM +0200, Kevin Daudt wrote:
> On Thu, Oct 12, 2017 at 02:02:17AM -0700, W. Trevor King wrote:
> > Pull has supported these since ea230d8 (pull: add the --gpg-sign
> > option, 2014-02-10).  Insert in long-option alphabetical order
> > following 7c85d274 (Documentation/merge-options.txt: order options in
> > alphabetical groups, 2009-10-22).
> > 
> > Signed-off-by: W. Trevor King <wk...@tremily.us>
> > ---
> > This patch is based on maint.  It will have trivial conflicts with the
> > --signoff docs which landed in 14d01b4f07 (merge: add a --signoff
> > flag, 2017-07-04, v2.15.0-rc0~138^2).
> > 
> >  Documentation/git-merge.txt | 6 --
> >  Documentation/merge-options.txt | 6 ++
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> > index f90faf7aaa..1d97a17904 100644
> > --- a/Documentation/git-merge.txt
> > +++ b/Documentation/git-merge.txt
> > @@ -64,12 +64,6 @@ OPTIONS
> >  ---
> >  include::merge-options.txt[]
> >  
> > --S[]::
> > ---gpg-sign[=]::
> 
> Shouldn't the options self be removed here too, not just the
> explanation?
> 

You can ignore this, it was just my mail client that colored the diff
wrong, confusing me.

> > -   GPG-sign the resulting merge commit. The `keyid` argument is
> > -   optional and defaults to the committer identity; if specified,
> > -   it must be stuck to the option without a space.
> > -
> >  -m ::
> > Set the commit message to be used for the merge commit (in
> > case one is created).
> > diff --git a/Documentation/merge-options.txt 
> > b/Documentation/merge-options.txt
> > index 5b4a62e936..6d85a76872 100644
> > --- a/Documentation/merge-options.txt
> > +++ b/Documentation/merge-options.txt
> > @@ -42,6 +42,12 @@ set to `no` at the beginning of them.
> > current `HEAD` is already up-to-date or the merge can be
> > resolved as a fast-forward.
> >  
> > +-S[]::
> > +--gpg-sign[=]::
> > +   GPG-sign the resulting merge commit. The `keyid` argument is
> > +   optional and defaults to the committer identity; if specified,
> > +   it must be stuck to the option without a space.
> > +
> >  --log[=]::
> >  --no-log::
> > In addition to branch names, populate the log message with
> > -- 
> > 2.13.6
> > 

Re: [PATCH] Documentation/merge-options.txt: Add -S/--gpg-sign

[Kevin Daudt]

On Thu, Oct 12, 2017 at 02:02:17AM -0700, W. Trevor King wrote:
> Pull has supported these since ea230d8 (pull: add the --gpg-sign
> option, 2014-02-10).  Insert in long-option alphabetical order
> following 7c85d274 (Documentation/merge-options.txt: order options in
> alphabetical groups, 2009-10-22).
> 
> Signed-off-by: W. Trevor King 
> ---
> This patch is based on maint.  It will have trivial conflicts with the
> --signoff docs which landed in 14d01b4f07 (merge: add a --signoff
> flag, 2017-07-04, v2.15.0-rc0~138^2).
> 
>  Documentation/git-merge.txt | 6 --
>  Documentation/merge-options.txt | 6 ++
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index f90faf7aaa..1d97a17904 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -64,12 +64,6 @@ OPTIONS
>  ---
>  include::merge-options.txt[]
>  
> --S[]::
> ---gpg-sign[=]::

Shouldn't the options self be removed here too, not just the
explanation?

> - GPG-sign the resulting merge commit. The `keyid` argument is
> - optional and defaults to the committer identity; if specified,
> - it must be stuck to the option without a space.
> -
>  -m ::
>   Set the commit message to be used for the merge commit (in
>   case one is created).
> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> index 5b4a62e936..6d85a76872 100644
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -42,6 +42,12 @@ set to `no` at the beginning of them.
>   current `HEAD` is already up-to-date or the merge can be
>   resolved as a fast-forward.
>  
> +-S[]::
> +--gpg-sign[=]::
> + GPG-sign the resulting merge commit. The `keyid` argument is
> + optional and defaults to the committer identity; if specified,
> + it must be stuck to the option without a space.
> +
>  --log[=]::
>  --no-log::
>   In addition to branch names, populate the log message with
> -- 
> 2.13.6
> 

Re: Unable to use --patch with git add

[Kevin Daudt]

On Wed, Oct 11, 2017 at 11:16:39PM +0530, Ayush Goel wrote:
> $ git --version
> git version 2.14.2
> 
> What more details can I provide to help debug this?
> 

Hello Ayush,

Run:

  git config --global color.ui auto
  git config --unset --local color.ui #in case it's set locally

This is a known 'breakage' because people have set color.ui to always
(which should not be used anyway).

Kind regards, Kevin

Re: [PATCH] column: show auto columns when pager is active

[Kevin Daudt]

On Wed, Oct 11, 2017 at 08:12:35PM +0200, Martin Ågren wrote:
> On 11 October 2017 at 19:23, Kevin Daudt <m...@ikke.info> wrote:
> > finalize_colopts in column.c only checks whether the output is a TTY to
> > determine if columns should be enabled with columns set to auto. Also check
> > if the pager is active.
> 
> Maybe you could say something about the difficulties of writing a test
> for `git column` proper. Something like this perhaps:
> 
>   Adding a test for git column is possible but requires some care to
>   work around a race on stdin. See commit 18d8c2693 (test_terminal:
>   redirect child process' stdin to a pty, 2015-08-04). Test git tag
>   instead, since that does not involve stdin, and since that was the
>   original motivation for this patch.

Right, makes sense.
> 
> > Helped-by: Rafael Ascensão <rafa.al...@gmail.com>
> > Signed-off-by: Kevin Daudt <m...@ikke.info>
> > ---
> >  column.c |  3 ++-
> >  t/t7006-pager.sh | 14 ++
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> Does the documentation on `column.ui` need to be updated? It talks about
> "if the output is to the terminal". That's similar to the documentation
> on the various `color.*`, so we should be fine, and arguably it's even
> better not to say anything since that makes it consistent.
> 
> > diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> > index f0f1abd1c..44c2ca5d3 100755
> > --- a/t/t7006-pager.sh
> > +++ b/t/t7006-pager.sh
> > @@ -570,4 +570,18 @@ test_expect_success 'command with underscores does not 
> > complain' '
> > test_cmp expect actual
> >  '
> >
> > +test_expect_success TTY 'git tag with auto-columns ' '
> > +   test_commit one &&
> > +   test_commit two &&
> > +   test_commit three &&
> > +   test_commit four &&
> > +   test_commit five &&
> > +   cat >expected <<\EOF &&
> > +initial  one  two  threefour five
> > +EOF
> > +   test_terminal env PAGER="cat >actual.tag" COLUMNS=80 \
> > +   git -p -c column.ui=auto tag --sort=authordate &&
> > +   test_cmp expected actual.tag
> > +'
> > +
> >  test_done
> 
> Since `git tag` pages when it's listing, you don't need the `-p`. But
> it's not like it hurts to have it. Yeah, I know, you needed it with `git
> column`. :-)

Right, it was a bit of a left-over since I assumed the PAGER='cat 
>paginated.out'
from the beginning of the test was in place and I wasn't getting any
output, but it turned out PAGER wasn't set.

> I wonder if it's useful to set COLUMNS a bit lower so that this has to
> split across more than one line (but not six), i.e., to do something
> non-trivial. I suppose that might lower the chances of some weird
> breakage slipping through.

Yeah, I was doubting about that, but wouldn't this amount to testing
whether git column is working properly, instead of just testing whether
it's being done at all?

> This test uses "actual.tag" while most (all?) others in this file use
> "actual". Maybe you worry about checking the "wrong" file, e.g., in case
> the pager doesn't kick in. You could do `rm -f actual` before the
> `test_terminal`-invocation to protect against that.

Yeah, I actually ran into that, but rm-ing it is better, I agree.

> These were just the thoughts that occurred to me, not sure if any of
> them is particularly significant. Thanks for cleaning up after me.
> 

np. Just as I posted earlier, I think you did not actually cause the bug
(because this has never worked), it just made it visible to more users.

Kevin

[PATCH] column: show auto columns when pager is active

[Kevin Daudt]

When columns are set to automatic for git tag and the output is
paginated by git, the output is a single column instead of multiple
columns.

Standard behaviour in git is to honor auto values when the pager is
active, which happens for example with commands like git log showing
colors when being paged.

Since ff1e72483 (tag: change default of `pager.tag` to "on",
2017-08-02), the pager has been enabled by default, exposing this
problem to more people.

finalize_colopts in column.c only checks whether the output is a TTY to
determine if columns should be enabled with columns set to auto. Also check
if the pager is active.

Helped-by: Rafael Ascensão <rafa.al...@gmail.com>
Signed-off-by: Kevin Daudt <m...@ikke.info>
---
 column.c |  3 ++-
 t/t7006-pager.sh | 14 ++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/column.c b/column.c
index ff7bdab1a..ded50337f 100644
--- a/column.c
+++ b/column.c
@@ -5,6 +5,7 @@
 #include "parse-options.h"
 #include "run-command.h"
 #include "utf8.h"
+#include "pager.c"
 
 #define XY2LINEAR(d, x, y) (COL_LAYOUT((d)->colopts) == COL_COLUMN ? \
(x) * (d)->rows + (y) : \
@@ -224,7 +225,7 @@ int finalize_colopts(unsigned int *colopts, int 
stdout_is_tty)
if (stdout_is_tty < 0)
stdout_is_tty = isatty(1);
*colopts &= ~COL_ENABLE_MASK;
-   if (stdout_is_tty)
+   if (stdout_is_tty || pager_in_use())
*colopts |= COL_ENABLED;
}
return 0;
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index f0f1abd1c..44c2ca5d3 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -570,4 +570,18 @@ test_expect_success 'command with underscores does not 
complain' '
test_cmp expect actual
 '
 
+test_expect_success TTY 'git tag with auto-columns ' '
+   test_commit one &&
+   test_commit two &&
+   test_commit three &&
+   test_commit four &&
+   test_commit five &&
+   cat >expected <<\EOF &&
+initial  one  two  threefour five
+EOF
+   test_terminal env PAGER="cat >actual.tag" COLUMNS=80 \
+   git -p -c column.ui=auto tag --sort=authordate &&
+   test_cmp expected actual.tag
+'
+
 test_done
-- 
2.14.2

Re: [RFC] column: show auto columns when pager is active

[Kevin Daudt]

On Tue, Oct 10, 2017 at 07:02:00PM +0200, Martin Ågren wrote:
> On 10 October 2017 at 16:01, Jeff King <p...@peff.net> wrote:
> > On Tue, Oct 10, 2017 at 12:30:49PM +0200, Martin Ågren wrote:
> >> On 9 October 2017 at 23:45, Kevin Daudt <m...@ikke.info> wrote:
> >> > Since ff1e72483 (tag: change default of `pager.tag` to "on",
> >> > 2017-08-02), the pager has been enabled by default, exposing this
> >> > problem to more people.
> >>
> >> Oh. :( I didn't know about "column" to be honest.
> >
> > Yeah, I didn't think of that with respect to the pager. This is a
> > regression in v2.14.2, I think.
> 
> Yes.

Though 2.14.2 enabled the pager by default, even before that when
someone would've enabled the pager theirselves (by setting pager.tag for
example), it would also shown just a single column.

I could reproduce it as far as 2.8.3 (I could not test further due to
libssl incompattibility).

Re: [RFC] column: show auto columns when pager is active

[Kevin Daudt]

On Tue, Oct 10, 2017 at 07:04:42PM +0200, Martin Ågren wrote:
> On 10 October 2017 at 16:29, Jeff King  wrote:
> > On Tue, Oct 10, 2017 at 10:10:19AM -0400, Jeff King wrote:
> >
> > it will randomly succeed or fail, depending on whether sed manages to
> > read the input before the stdin terminal is closed.
> >
> > I'm not sure of an easy way to fix test-terminal, but we could work
> > around it like this (which also uses "-p" to actually invoke the pager,
> > and uses a pager that makes it clear when it's being run):
> >
> > diff --git a/t/t9002-column.sh b/t/t9002-column.sh
> > index 9441145bf0..d322c3b745 100755
> > --- a/t/t9002-column.sh
> > +++ b/t/t9002-column.sh
> > @@ -180,14 +180,14 @@ EOF
> >
> >  test_expect_success TTY '20 columns, mode auto, pager' '
> > cat >expected <<\EOF &&
> > -oneseven
> > -twoeight
> > -three  nine
> > -four   ten
> > -five   eleven
> > -six
> > +paged:oneseven
> > +paged:twoeight
> > +paged:three  nine
> > +paged:four   ten
> > +paged:five   eleven
> > +paged:six
> >  EOF
> > -   test_terminal env PAGER="cat|cat" git column --mode=auto  > >actual &&
> > +   test_terminal env PAGER="sed s/^/paged:/" sh -c "git -p column 
> > --mode=auto actual &&
> > test_cmp expected actual
> >  '
> >  test_done
> 
> Makes sense. FWIW, I don't see the flakyness with this.
> 
> > All that said, I think I'd just as soon test a real command like "git
> > tag", which doesn't care about reading from stdin.
> 
> For reference for Kevin, in case you consider testing, e.g., git tag,
> the main reason I referred to the test I posted as "hacky", was that I
> just inserted it at a more-or-less random place in t7006. So it had to
> play with `git reset` to avoid upsetting the later tests. It could
> obviously go to the end instead, but I was too lazy to move it and
> define a pager.

Thanks Jeff and Martin, I will use your tips to build a test based on
git tag instead.

[RFC] column: show auto columns when pager is active

[Kevin Daudt]

When columns are set to automatic for git tag and the output is
paginated by git, the output is a single column instead of multiple
columns.

Standard behaviour in git is to honor auto values when the pager is
active, which happens for example with commands like git log showing
colors when being paged.

Since ff1e72483 (tag: change default of `pager.tag` to "on",
2017-08-02), the pager has been enabled by default, exposing this
problem to more people.

finalize_colopts in column.c only checks whether the output is a TTY to
determine if columns should be enabled with columns set to autol. Also check
if the pager is active.

Helped-by: Rafael Ascensão <rafa.al...@gmail.com>
Signed-off-by: Kevin Daudt <m...@ikke.info>
---
This came to light when someone wondered on irc why
column.tag=[auto|always] did not work on Mac OS. Testing it myself, I
found it to work with always, but not with auto.

I could not get the test to work yet, because somehow it's not giving
any output, so feedback regarding that is welcome.


 column.c  |  3 ++-
 t/t9002-column.sh | 13 +
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/column.c b/column.c
index ff7bdab1a..ded50337f 100644
--- a/column.c
+++ b/column.c
@@ -5,6 +5,7 @@
 #include "parse-options.h"
 #include "run-command.h"
 #include "utf8.h"
+#include "pager.c"
 
 #define XY2LINEAR(d, x, y) (COL_LAYOUT((d)->colopts) == COL_COLUMN ? \
(x) * (d)->rows + (y) : \
@@ -224,7 +225,7 @@ int finalize_colopts(unsigned int *colopts, int 
stdout_is_tty)
if (stdout_is_tty < 0)
stdout_is_tty = isatty(1);
*colopts &= ~COL_ENABLE_MASK;
-   if (stdout_is_tty)
+   if (stdout_is_tty || pager_in_use())
*colopts |= COL_ENABLED;
}
return 0;
diff --git a/t/t9002-column.sh b/t/t9002-column.sh
index 89983527b..9441145bf 100755
--- a/t/t9002-column.sh
+++ b/t/t9002-column.sh
@@ -2,6 +2,7 @@
 
 test_description='git column'
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
 
 test_expect_success 'setup' '
cat >lista <<\EOF
@@ -177,4 +178,16 @@ EOF
test_cmp expected actual
 '
 
+test_expect_success TTY '20 columns, mode auto, pager' '
+   cat >expected <<\EOF &&
+oneseven
+twoeight
+three  nine
+four   ten
+five   eleven
+six
+EOF
+   test_terminal env PAGER="cat|cat" git column --mode=auto actual 
&&
+   test_cmp expected actual
+'
 test_done
-- 
2.14.2

Re: "git rm" seems to do recursive removal even without "-r"

[Kevin Daudt]

On Sun, Oct 08, 2017 at 05:07:12AM -0400, Robert P. J. Day wrote:
> On Sun, 8 Oct 2017, Junio C Hamano wrote:
> 
> > "Robert P. J. Day"  writes:
> >
> > > ... so if, in the kernel source
> > > tree, i ran:
> > >
> > >   $ git rm \*.c
> > >
> > > i would end up removing *all* 25,569 "*.c" files in the kernel source
> > > repository.
> >
> > Yes, as that is exactly what the command line asks Git to do.
> 
>   so if i wanted git to remove, say, all files named "Makefile*" from
> everywhere in the linux kernel source tree, the (dry run) command
> would be:
> 
>   $ git rm -n Makefile\*
> 
> is that it? let's try that:
> 
>   $ git rm -n Makefile\*
>   rm 'Makefile'
>   $
> 
> oops.
> 
> rday
> 

So your question is not su much  about the recursive option (delete
mentioned directories, including their contents), but globbing
(expanding the * to any files matching the pattern).

The explanation of  mentions this:

   Files to remove. Fileglobs (e.g.  *.c) can be given to remove all
   matching files.

This indicates that git itself (not your shell alone) does file
globbing.

I think the confusing part is that most people have no clear idea of the
separation between what the shell sees and interprets, and what the
program actually gets.

When you execute:

$ git rm Makefile\*

What git actually sees is this:

Makefile*

The shell intepreted the \* to mean just '*' and not interpret it
itself, and provide that to the executed program. Git, in its turn,
would start matching any file to that pattern to see which files
matches.


If you would execute:

$ git rm 'Makefile\*'

Git would see:

Makefile\*

Which does the thing you intended. So you have to deal with 2 levels of
programs interpreting the arguments, not just one.

Whether '*.c' should match just all .c files in the current dir, or all
files ending with .c depends on whether the path seperator is matched by
* or not and is a separate discussion.

GITGLOSSARY(7) under pathspec mentions this:

globGit treats the pattern as a shell glob suitable for consumption
by fnmatch(3) with the FNM_PATHNAME flag: wildcards in the
pattern will not match a / in the pathname.

So that seems to indicate '*.c' should only match .c files in the
current dir. I'm not sure why that's not the case.

I hope this clears up what's happening a bit, and perhaps can lead to
improvements to the documentation so that it's not so surprising.

Kind regards, Kevin.

Re: [bug] git add -p breaks, if color.ui is set to "always"

[Kevin Daudt]

On Fri, Oct 06, 2017 at 02:47:30PM +0200, Alexander Gehrke wrote:
> After an update to version 2.14.2 from 2.14.1 "git add --patch" stopped 
> working
> for me, just producing the same output as "git diff", but not prompting to 
> stage
> anything.
> 
> I found that unsetting the config key color.ui, which was set to "always" 
> fixed
> the problem.
> 
> From the manpage, color.ui should not have that effect and "always" should be 
> a
> legal value.
> 
> Regards
> Alexander Gehrke

Hello Alexander,

There have been a few mailing-list posts[0] about this already. While
git add -p should probably not have broken by this, setting ui.color to
always itself does not make a lot of sense either.

You are telling git to always output color, even when the target is
something that does not know what to do with the color codes. Setting it
to 'auto' would make more sense.

The thread I posted to discusses some changes that might get introduced
to improve the situation though.

Kevin.

[0}:https://public-inbox.org/git/20171003093157.gq7za2fwcqsou...@sigill.intra.peff.net/T/

Re: distinguishing between staged and unstaged content in a stash?

[Kevin Daudt]

On Wed, Oct 04, 2017 at 07:10:46AM -0400, rpj...@crashcourse.ca wrote:
> 
>   couple (admittedly trivial) questions about stashing. first, can i
> clarify that when one stashes content, a stash *always* distinguishes
> between what was staged, and what was unstaged? that is, when one is
> stashing, the "--keep-index" option relates to whether or not staged
> changes are left in the index (and, consequently, in the working
> directory as well), but that option has no effect on the final content
> of the stash, yes? even if "--keep-index" is used, staged content
> still ends up in the stash.
> 
>   also, is there a simple way to distinguish between the staged and
> unstaged contents of a stash (or, more basically, is this even a
> useful question to ask)? out of curiosity, i tried to figure out
> how to do this, and came up with the following.
> 
> to see staged portion of stash@{0}:
> 
>   $ git show stash@{0}^2
> 
> to see unstaged portion:
> 
>   $ git diff stash@{0}^2 stash@{0}
> 
> it's not like i have a pressing need to do that, i was just curious
> if there's a simpler way to do this, or if this is just not something
> people should need to do on a regular basis.
> 
> rday
> 
> 
> 

There was a recent thread about this[0]. The conclusion is that it's
seen as a good change, someone just needs to supply the patch to do
this. A possible solution was also provided (from before that thread)
here [1]

[0]:https://public-inbox.org/git/1505626069.9625.6.ca...@gmail.com/
[1]:https://public-inbox.org/git/20170317141417.g2oenl67k74nl...@sigill.intra.peff.net/

Re: git add -p stops working when setting color.ui = always

[Kevin Daudt]

On Mon, Oct 02, 2017 at 01:38:17PM +0300, Tsvi Mostovicz wrote:
> Hi,
> 
> When setting "color.ui = always" in the last few versions (not sure
> exactly when this started, but definitely exists in 2.14.1 and
> 2.14.2), git add -p stops working as expected. Instead of prompting
> the user, it skips through the prompts and doesn't allow selecting a
> hunk.
> 
> Don't know why I had color.ui = always set in my .gitconfig.
> 
> git version 2.14.2.666.gea220ee40
> 
> Thanks,
> 

Hello Tsvi,

This is being discussed just now[0]. Setting the value to auto should
fix it (setting it to always does not make much sense in your config
file).

Kevin.

[0}:https://public-inbox.org/git/20171003093157.gq7za2fwcqsou...@sigill.intra.peff.net/T/

Re: will git rebase has side effect

[Kevin Daudt]

On Mon, Oct 02, 2017 at 12:06:38AM +0800, Yubin Ruan wrote:
> 2017-10-01 22:17 GMT+08:00 Kevin Daudt <m...@ikke.info>:
> > Forgot to cc the mailing list.
> >
> > On Sun, Oct 01, 2017 at 09:23:23PM +0800, Yubin Ruan wrote:
> >> Suppose that I have such a history of commit locally:
> >>
> >> A --> B --> C --> D
> >>
> >> If I then add a few more commits locally
> >>
> >> A --> B --> C --> D --> E --> F --> G
> >>
> >> And then I do a rebase and squash F and G into one single commit H.
> >> What side effect will this rebase have? How will this affect "git push
> >> origin master"?
> >>
> >> Yubin
> >
> > Hello Yubin,
> >
> > So the situation is this:
> >
> > [origin/master]
> >   |
> > A --> B --> C --> D --> E --> F --> G
> > |
> >  [master]
> >
> > Then you squash (F' is the result of squashing F and G):
> >
> > [origin/master]
> >   |
> > A --> B --> C --> D --> E --> F'
> >   |
> >[master]
> >
> > When you want to push now, it's just as if you just created just two
> > commits in the first place, and you can just push normally (assuming no
> > one else has pushed in the mean time.
> 
> Hmm..You mean, if I do a squash, it will only affects those commits
> that has been squashed, not any other commits, and their parent-child
> relations remain the same?
> 
> Yubin

Only the commits being squashed, and the commits after it (not
applicable in your case). But not commits that come before the squash.
Remember that in git, commits point at their parent(s), not the opposite
way. So if you change commits, only the children will have to change (to
point to the new hashes), but not their parents.

Re: will git rebase has side effect

[Kevin Daudt]

Forgot to cc the mailing list.

On Sun, Oct 01, 2017 at 09:23:23PM +0800, Yubin Ruan wrote:
> Suppose that I have such a history of commit locally:
> 
> A --> B --> C --> D
> 
> If I then add a few more commits locally
> 
> A --> B --> C --> D --> E --> F --> G
> 
> And then I do a rebase and squash F and G into one single commit H.
> What side effect will this rebase have? How will this affect "git push
> origin master"?
> 
> Yubin

Hello Yubin,

So the situation is this:

[origin/master]
  |
A --> B --> C --> D --> E --> F --> G
|
 [master] 

Then you squash (F' is the result of squashing F and G):

[origin/master]
  |
A --> B --> C --> D --> E --> F'
  |
   [master] 

When you want to push now, it's just as if you just created just two
commits in the first place, and you can just push normally (assuming no
one else has pushed in the mean time.

The situation is different when you have pushed already:

  [origin/master]
|
A --> B --> C --> D --> E --> F --> G
|
 [master] 

Then after you squash, the history would look as follows:

  [origin/master]
|
A --> B --> C --> D --> E --> F --> G
 \
  --> F'
  |
   [master] 

This sitation would look to git like you created F', and someone else
had pushed F and G. It will reject the push, saying you would need to
merge these changes first (but you don't want this, because you are
merging the same changes together).

To solve this, you can use git push --force-with-lease=master origin,
which will force the push, overwriting the history the remote already
had.

Hope this helps, Kevin.

nb. --force-with-lease is a safer version of just (-f|--force), because
it will prevent you from overwriting history you don't have locally, for
example when someone else did push already.

Re: "man git-checkout", man page is inconsistent between SYNOPSIS and details

[Kevin Daudt]

On Sat, Sep 30, 2017 at 05:27:22AM -0400, Robert P. J. Day wrote:
> 
>   just noticed that in "man git-checkout", the SYNOPSIS contains the
> line:
> 
>   git checkout [-p|--patch] [] [--] [...]
> 
> implying that  is optional, but further down in the
> DESCRIPTION, one reads:
> 
>   git checkout [-p|--patch] [] [--] ...
> 
> suggesting that  is required.
> 

Hello Robert, thank you for this report.

Git checkout has 2 major modes of operating:

1. Checking out branches (and then update your working tree to match that
  commit.
2. Checking out 1 or more files from a commit.

The first four lines of the synopsis match mode nr. 1. The next two
belong to mode nr. 2.

The pathspec in the synopsis line you are quoting is required, because
that's how you tell git you want mode nr 2. That's why it's not
mentioned between [].  The last section under description explains that
mode. 

Do you feel this distinction needs to be made more clear?

Kevin.

Re: '--shallow-since' option is not available for git-pull in Git version 2.14.1

[Kevin Daudt]

On Mon, Sep 25, 2017 at 04:31:10PM +0900, Sanggyu Nam wrote:
> I’ve found that some subcommands such as git-clone, git-fetch, and
> git-pull support an option named ‘--shallow-since’, as of Git version
> 2.11. This option is documented in the man page of each subcommand. In
> Git 2.14.1, I’ve checked that the option is available for git-clone
> and git-fetch so that the history of a shallow repository is updated.
> However, running git-pull with this option shows an error as follows:
> 
> error: unknown option `shallow-since=2017-09-10T00:00:00+00:00'
> 
> usage: git pull [] [ [...]]
> ...
> 
> I found that this option is not available in Git 2.14.1 on macOS and
> Ubuntu 16.04.1. It seems the option handling of git-pull does not
> match with what’s described in the man page. Since ‘git pull’ is a
> shorthand for ‘git fetch’ followed by ‘git merge FETCH_HEAD’ in its
> default mode, we can run these two commands in this order as a
> workaround.
> 
> 

This does not only count for --shallow-since, but also --deepen
and --shallow-exclude.

Re: [PATCH] t2018: remove unwanted empty line

[Kevin Daudt]

On Sun, Sep 17, 2017 at 10:19:28AM +, Kaartic Sivaraam wrote:
> Signed-off-by: Kaartic Sivaraam 
> ---
>  t/t2018-checkout-branch.sh | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
> index 2131fb2a5682c..e0a52334a22dd 100755
> --- a/t/t2018-checkout-branch.sh
> +++ b/t/t2018-checkout-branch.sh
> @@ -192,7 +192,6 @@ test_expect_success 'checkout -b ' '
>  test_expect_success 'checkout -B to the current branch works' '
>   git checkout branch1 &&
>   git checkout -B branch1-scratch &&
> -
>   setup_dirty_mergeable &&
>   git checkout -B branch1-scratch initial &&
>   test_dirty_mergeable
> 
> --
> https://github.com/git/git/pull/403

Why is this empty line unwanted? This kind of whitespace can help
separate logical sections, just like paragraphs would.

Kevin.

Re: Git 2.14.1: t6500: error during test on musl libc

[Kevin Daudt]

On Fri, Sep 15, 2017 at 08:37:40AM +0200, Kevin Daudt wrote:
> On Thu, Sep 14, 2017 at 09:43:12PM -0500, A. Wilcox wrote:
> > -BEGIN PGP SIGNED MESSAGE-
> > Hash: SHA256
> > 
> > Hi there,
> > 
> > While bumping Git's version for our Linux distribution to 2.14.1, I've
> > run in to a new test failure in t6500-gc.sh.  This is the output of
> > the failing test with debug=t verbose=t:
> 
> This is a new test introduced by c45af94db 
> (gc: run pre-detach operations under lock, 2017-07-11) which was
> included in v2.14.0.
> 
> So it might be that this was already a problem for a longer time, only
> just recently uncovered.
> 

Adding Jeff King to CC

Re: Git 2.14.1: t6500: error during test on musl libc

[Kevin Daudt]

On Thu, Sep 14, 2017 at 09:43:12PM -0500, A. Wilcox wrote:
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA256
> 
> Hi there,
> 
> While bumping Git's version for our Linux distribution to 2.14.1, I've
> run in to a new test failure in t6500-gc.sh.  This is the output of
> the failing test with debug=t verbose=t:

This is a new test introduced by c45af94db 
(gc: run pre-detach operations under lock, 2017-07-11) which was
included in v2.14.0.

So it might be that this was already a problem for a longer time, only
just recently uncovered.

> 
> 
> expecting success:
> # make sure we run a background auto-gc
> test_commit make-pack &&
> git repack &&
> test_config gc.autopacklimit 1 &&
> test_config gc.autodetach true &&
> 
> # create a ref whose loose presence we can use to detect a
> pack-refs run
> git update-ref refs/heads/should-be-loose HEAD &&
> test_path_is_file .git/refs/heads/should-be-loose &&
> 
> # now fake a concurrent gc that holds the lock; we can use our
> # shell pid so that it looks valid.
> hostname=$(hostname || echo unknown) &&
> printf "$$ %s" "$hostname" >.git/gc.pid &&
> 
> # our gc should exit zero without doing anything
> run_and_wait_for_auto_gc &&
> test_path_is_file .git/refs/heads/should-be-loose
> 
> [master 28ecdda] make-pack
>  Author: A U Thor 
>  1 file changed, 1 insertion(+)
>  create mode 100644 make-pack.t
> Counting objects: 3, done.
> Delta compression using up to 8 threads.
> Compressing objects: 100% (2/2), done.
> Writing objects: 100% (3/3), done.
> Total 3 (delta 0), reused 0 (delta 0)
> Auto packing the repository in background for optimum performance.
> See "git help gc" for manual housekeeping.
> File .git/refs/heads/should-be-loose doesn't exist.
> not ok 8 - background auto gc respects lock for all operations
> #
> #   # make sure we run a background auto-gc
> #   test_commit make-pack &&
> #   git repack &&
> #   test_config gc.autopacklimit 1 &&
> #   test_config gc.autodetach true &&
> #
> #   # create a ref whose loose presence we can use to
> detect a pack-refs run
> #   git update-ref refs/heads/should-be-loose HEAD &&
> #   test_path_is_file .git/refs/heads/should-be-loose &&
> #
> #   # now fake a concurrent gc that holds the lock; we can
> use our
> #   # shell pid so that it looks valid.
> #   hostname=$(hostname || echo unknown) &&
> #   printf "$$ %s" "$hostname" >.git/gc.pid &&
> #
> #   # our gc should exit zero without doing anything
> #   run_and_wait_for_auto_gc &&
> #   test_path_is_file .git/refs/heads/should-be-loose
> #
> 
> # failed 1 among 8 test(s)
> 1..8
> 
> 
> I admit I am mostly blind with the Git gc system.  Should I use strace
> on the git-gc process at the end?  How would I accomplish that?  Is
> there a better way of debugging this error further?
> 
> Core system stats:
> 
> Intel x86_64 E3-1280 v3 @ 3.60 GHz
> musl libc 1.1.16+20
> git 2.14.1, vanilla except for a patch to an earlier test due to
> musl's inability to cope with EUC-JP
> bash 4.3.48(1)-release
> 
> Thank you very much.
> 
> All the best,
> - --arw
> 
> - -- 
> A. Wilcox (awilfox)
> Project Lead, Adélie Linux
> http://adelielinux.org
> -BEGIN PGP SIGNATURE-
> Version: GnuPG v2
> 
> iQIcBAEBCAAGBQJZuz4yAAoJEMspy1GSK50UORwP/0Jxfp3xzexh27tSJlXYWS/g
> g9QK8Xmid+3A0R696Vb2GguKg2roCcTmM2anR7iD1B2f2W31sgf+8M5mnJRHyJ1p
> geEeqwrTdpCk6jQ/1Pj03L0NOftb1ftR6hcoVujBFAOph4jRlRdZDPA87fe6snrh
> q99C3LoDXQcyK6WWJwzX+t2wOplKgpGJP8wTAaZ0AHoUwVS5CLPl8tP2XaY4kLfD
> ZPPcvtp9wisVzzZ2ssE/CLGd38EbenNNZ6OJCBFJIHmlwey4G2isZ9kk6fVIHXi2
> unBJ8yVqI7hQKmQFSVQMMSFSd9azhHnDjTBO5mzWeRK9HNVMda3LZsXTtVeswnRs
> lN/ASMdt5KdfpNy/plFB7yDWLlQSQY7j1mxBMR8lL3AdVVQUbJppDM795tt+rn6a
> NCE2ESZMWd/QEULmT92AbkNJTj5ibBEoubnVTka05KMjaBLwIauhpqU5XxLFq2UH
> y3JYQU9hm0E7dQE0CLXxIm5/574T6bBUgp1cXH3CjxkeUYKR1USVKtDfBV6t/Qmt
> xlDZKPEfjKbTvL3KUF33G+eAp55wTwrJTaWlOp8A/JqooXavYghcsuFhYtCPJ8qo
> fFUa8kBZP70E/O7JkycUu8wi7p42+j1a8gR6/AnPG2u2wyoiosLCxHX+nll4gKmN
> b6BuiRn0Z9ie5xw4xcMR
> =Vf8Z
> -END PGP SIGNATURE-

Re: [PATCH] commit-template: change a message to be more intuitive

[Kevin Daudt]

On Tue, Sep 12, 2017 at 04:25:36PM +0530, Kaartic Sivaraam wrote:
> It's not possible to 'touch' the cut-line that is shown when the
> user requests a patch in his commit template.
> 

Touching something can also mean to disturb or change something, which
is the meaning being used here, so it is not an incorrect use of the
word.

> So, make the sentence more intuitive.

I can understand however that it might be not so clear for people with
less fluency in English.

> 
> Signed-off-by: Kaartic Sivaraam 
> ---
>  Just a small tweak. May or may not be worth the patch.
> 
>  wt-status.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/wt-status.c b/wt-status.c
> index 77c27c511..1f54b1df2 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -934,7 +934,7 @@ size_t wt_status_locate_end(const char *s, size_t len)
>  
>  void wt_status_add_cut_line(FILE *fp)
>  {
> - const char *explanation = _("Do not touch the line above.\nEverything 
> below will be removed.");
> + const char *explanation = _("Do not edit the line above.\nEverything 
> below will be removed.");
>   struct strbuf buf = STRBUF_INIT;
>  
>   fprintf(fp, "%c %s", comment_line_char, cut_line);
> -- 
> 2.14.1.1006.g90ad9a07c
> 

Re: [PATCH v2 1/2] doc/for-each-ref: consistently use '=' to between argument names and values

[Kevin Daudt]

On Tue, Sep 12, 2017 at 11:28:00AM +0900, Junio C Hamano wrote:
> Kevin Daudt <m...@ikke.info> writes:
> 
> > The synopsis and description inconsistently add a '=' between the
> > argument name and it's value. Make this consistent.
> >
> > Signed-off-by: Kevin Daudt <m...@ikke.info>
> > ---
> >  Documentation/git-for-each-ref.txt | 15 ---
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> Good idea, and I think it is in line with an earlier suggestion by
> Jonathan (cc'ed).
> 
> Thanks.
> 

Yeah, this is his diff applied. Forgot to CC him.

[PATCH v2 2/2] doc/for-each-ref: explicitly specify option names

[Kevin Daudt]

For count, sort and format, only the argument names were listed under
OPTIONS, not the option names.

Add the option names to make it clear the options exist

Signed-off-by: Kevin Daudt <m...@ikke.info>
---
 Documentation/git-for-each-ref.txt | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 1015c88f6..66b4e0a40 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -26,19 +26,25 @@ host language allowing their direct evaluation in that 
language.
 
 OPTIONS
 ---
-::
+...::
+   If one or more patterns are given, only refs are shown that
+   match against at least one pattern, either using fnmatch(3) or
+   literally, in the latter case matching completely or from the
+   beginning up to a slash.
+
+--count=::
By default the command shows all refs that match
``.  This option makes it stop after showing
that many refs.
 
-::
+--sort=::
A field name to sort on.  Prefix `-` to sort in
descending order of the value.  When unspecified,
`refname` is used.  You may use the --sort= option
multiple times, in which case the last key becomes the primary
key.
 
-::
+--format=::
A string that interpolates `%(fieldname)` from a ref being shown
and the object it points at.  If `fieldname`
is prefixed with an asterisk (`*`) and the ref points
@@ -51,12 +57,6 @@ OPTIONS
`xx`; for example `%00` interpolates to `\0` (NUL),
`%09` to `\t` (TAB) and `%0a` to `\n` (LF).
 
-...::
-   If one or more patterns are given, only refs are shown that
-   match against at least one pattern, either using fnmatch(3) or
-   literally, in the latter case matching completely or from the
-   beginning up to a slash.
-
 --shell::
 --perl::
 --python::
-- 
2.14.1.459.g238e487ea9

[PATCH v2 1/2] doc/for-each-ref: consistently use '=' to between argument names and values

[Kevin Daudt]

The synopsis and description inconsistently add a '=' between the
argument name and it's value. Make this consistent.

Signed-off-by: Kevin Daudt <m...@ikke.info>
---
 Documentation/git-for-each-ref.txt | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index bb370c9c7..1015c88f6 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -10,8 +10,9 @@ SYNOPSIS
 [verse]
 'git for-each-ref' [--count=] [--shell|--perl|--python|--tcl]
   [(--sort=)...] [--format=] [...]
-  [--points-at ] [(--merged | --no-merged) []]
-  [--contains []] [--no-contains []]
+  [--points-at=]
+  (--merged[=] | --no-merged[=])
+  [--contains[=]] [--no-contains[=]]
 
 DESCRIPTION
 ---
@@ -65,24 +66,24 @@ OPTIONS
the specified host language.  This is meant to produce
a scriptlet that can directly be `eval`ed.
 
---points-at ::
+--points-at=::
Only list refs which points at the given object.
 
---merged []::
+--merged[=]::
Only list refs whose tips are reachable from the
specified commit (HEAD if not specified),
incompatible with `--no-merged`.
 
---no-merged []::
+--no-merged[=]::
Only list refs whose tips are not reachable from the
specified commit (HEAD if not specified),
incompatible with `--merged`.
 
---contains []::
+--contains[=]::
Only list refs which contain the specified commit (HEAD if not
specified).
 
---no-contains []::
+--no-contains[=]::
Only list refs which don't contain the specified commit (HEAD
if not specified).
 
-- 
2.14.1.459.g238e487ea9

[PATCH] doc/for-each-ref: explicitly specify option names

[Kevin Daudt]

For count, sort and format, only the argument names were listed under
OPTIONS, not the option names.

Add the option names to make it clear the options exist

Signed-off-by: Kevin Daudt <m...@ikke.info>
---
 Documentation/git-for-each-ref.txt | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index bb370c9c7..0c2032855 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -25,19 +25,25 @@ host language allowing their direct evaluation in that 
language.
 
 OPTIONS
 ---
-::
+...::
+   If one or more patterns are given, only refs are shown that
+   match against at least one pattern, either using fnmatch(3) or
+   literally, in the latter case matching completely or from the
+   beginning up to a slash.
+
+--count ::
By default the command shows all refs that match
``.  This option makes it stop after showing
that many refs.
 
-::
+--sort ::
A field name to sort on.  Prefix `-` to sort in
descending order of the value.  When unspecified,
`refname` is used.  You may use the --sort= option
multiple times, in which case the last key becomes the primary
key.
 
-::
+--format ::
A string that interpolates `%(fieldname)` from a ref being shown
and the object it points at.  If `fieldname`
is prefixed with an asterisk (`*`) and the ref points
@@ -50,12 +56,6 @@ OPTIONS
`xx`; for example `%00` interpolates to `\0` (NUL),
`%09` to `\t` (TAB) and `%0a` to `\n` (LF).
 
-...::
-   If one or more patterns are given, only refs are shown that
-   match against at least one pattern, either using fnmatch(3) or
-   literally, in the latter case matching completely or from the
-   beginning up to a slash.
-
 --shell::
 --perl::
 --python::
-- 
2.14.1.459.g238e487ea9

Re: Bug report

[Kevin Daudt]

On Wed, Aug 30, 2017 at 11:25:00PM +0200, Aleksandar Pavic wrote:
>  I have a file
> 
>  app/Controller/CustomerCardVerificationController.php
> 
> And when I take a look at changes log, I get this (no matter which tool I
> use):
> 
> 2017-07-31 19:41 dule o membership renew payment email
> 2017-06-07 08:59 Dusan Tatic  o cc refund clean
> 2017-04-15 00:16 Miodrag Dragić   o refound admin payment
> 2017-03-20 12:02 Dusan Tatic  o CardVerification card connect
> 2017-03-16 15:59 Aleksandar Pavic o paypal
> 2017-03-10 13:34 Aleksandar Pavic o Production branch
> 2017-03-10 13:01 Aleksandar Pavic I Migrating dev
> 
> However if I manually browse thru revisions and open revision from
> 03/27/2017 07:05 PM
> 
> I can see the change in that file which is unlisted above, at revision
> ff9f4946e109bd234d438e4db1d319b1f6cb6580
> 
> And I'm at master branch all the time...
> 
> We wouldn't have noticed that if it weren't one important feature...
> 

What does git branch --contains ff9f4946e109bd234d438e4db1d319b1f6cb6580
return?

Where did you find this commit?

Re: [RFC 5/7] http: send Git-Protocol-Version header

[Kevin Daudt]

On Thu, Aug 24, 2017 at 03:53:26PM -0700, Brandon Williams wrote:
> Tell a serve that protocol v2 can be used by sending an http header
> indicating this.

s/serve/server/

Re: What does 'git blame --before -- ' supposed to mean?

[Kevin Daudt]

On Mon, Aug 21, 2017 at 12:15:59AM +0530, Kaartic Sivaraam wrote:
> Hello all,
> 
> I tried to do a 'git blame' on a file and wanted to see the blame
> before a particular revision of that file. I initially didn't know that
> you could achieve that with,
> 
> $ git blame  
> 
> I thought I need to pass the revision as a parameter so I tried (before
> looking into the documentation) the command found in the subject. It
> worked but to my surprise it had the same result as,
> 
> $ git blame -- 
> 
> I was confused and came to know from the documentation that blame
> doesn't have any '--before' option. That was even more surprising. Why
> does blame accept an option which it doesn't identify? Shouldn't it
> have warned that it doesn't accept the '--before' option? I guess it
> should not accept it because it confuses the user a lot as the could
> make it hard time for him to identify the issue.
> 
> 'git blame' doesn't seem to be the only command that accepts options
> not specified in the documentation there's 'git show' for it's company,
> 
> $ git show --grep 'regex'
> 
> But the good thing with the above command is it behaves as expected. I
> suspect this should be documented, anyway.
> 
> Thoughts ?
> 
> -- 
> Kaartic

Git blame takes options that are fed to git rev-list, to limit the
commits being taken into account for blaming.

The man page shows "[--since=]", which is one such option, but
before is valid as well.

git blame -h shows:

 are documented in git-rev-list(1) 

and man git-blame shows under specifying ranges (emphasis mine): 

 When you are not interested in changes older than version v2.6.18,
 or changes older than 3 weeks, *you can use revision range
 specifiers similar to git rev-list*:

So these options are not documented under git blame, but git rev-list.

Perhaps the synopsis of man git-blame could be expanded so that that
it's clear it accepts rev-list options.

Kevin

Re: Undocumented change in `git branch -M` behavior

[Kevin Daudt]

On Wed, Aug 23, 2017 at 01:13:34PM -0700, Nish Aravamudan wrote:
> Hello,
> 
> Hopefully, I've got this right -- I noticed a change in behavior in git
> with Ubuntu 17.10, which recently got 2.14.1. Specifically, that when in
> an orphaned branch, -M ends up moving HEAD to the new branch name,
> clobbering the working tree. As far as I know, from the manpages,
> orphaned branches are still supported and should work?
> 
> I think an example will demonstrate more than words (the following are
> done in LXD containers, hence the root user):
> 
> # git --version
> git version 2.14.1
> # mkdir test && cd test && git init .
> Initialized empty Git repository in /root/test/.git/
> # git checkout -b a
> Switched to a new branch 'a'
> # touch testfile && git add testfile && git commit -m 'initial commit'
> [a (root-commit) 6061193] initial commit
>  Committer: root 
>  1 file changed, 0 insertions(+), 0 deletions(-)
>  create mode 100644 testfile
> # git checkout --orphan master
> Switched to a new branch 'master'
> # git status
> On branch master
> 
> No commits yet
> 
> Changes to be committed:
>   (use "git rm --cached ..." to unstage)
> 
> new file:   testfile
> 
> # git reset --hard && git status
> On branch master
> 
> No commits yet
> 
> nothing to commit (create/copy files and use "git add" to track)
> # git branch -M a b
> # git status
> On branch b
> Changes to be committed:
>   (use "git reset HEAD ..." to unstage)
> 
> deleted:testfile
> 
> This is very unexpected. I force-renamed a branch I wasn't currently
> checked out to and now I'm checked out to it *and* I have staged file
> removals (I think what is effectively happening is my current working
> directory (empty) is being staged into the new branch, but I'm not
> 100%).
> 
> For comparision, on 17.04:
> 
> # git --version
> git version 2.11.0
> # mkdir test && cd test && git init .
> Initialized empty Git repository in /root/test/.git/
> # git checkout -b a
> Switched to a new branch 'a'
> # touch testfile && git add testfile && git commit -m 'initial commit'
> [a (root-commit) f8d0d53] initial commit
>  Committer: root 
>  1 file changed, 0 insertions(+), 0 deletions(-)
>  create mode 100644 testfile
> # git checkout --orphan master
> Switched to a new branch 'master'
> # git status
> On branch master
> 
> No commits yet
> 
> Changes to be committed:
>   (use "git rm --cached ..." to unstage)
> 
> new file:   testfile
> 
> # git reset --hard && git status
> On branch master
> 
> No commits yet
> 
> nothing to commit (create/copy files and use "git add" to track)
> # git branch -M a b
> # git status
> On branch master
> 
> Initial commit
> 
> nothing to commit (create/copy files and use "git add" to track)
> 
> This is what I expect to see, the branch rename has no effect on HEAD.
> 
> I haven't yet bisected this (but I can if necessary). My initial
> suspicion is
> https://github.com/git/git/commit/70999e9ceca47e03b8900bfb310b2f804125811e#diff-d18f86ea14e2f1e5bff391b2e54438cb
> where a comparison between the oldname of the branch and HEAD was
> performed before attempting to move HEAD (so that HEAD followed to the
> new branch name, I believe). That change was dropped, though and perhaps
> the new check in replace_each_worktree_head_symref of
> 
> strcmp(oldref, worktrees[i]->head_ref)
> 
> does not work for orphaned branches? I am unfamiliar with all the
> details of the git internals, so please correct me if I'm wrong!
> 
> Thanks,
> Nish
> 
> -- 
> Nishanth Aravamudan
> Ubuntu Server
> Canonical Ltd

Thanks for this report. I've bisected it down to 
fa099d232 (worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe(), 
2017-04-24) 

I've CC'ed Duy, who made that commit.

Kevin

Re: How to force a push to succeed?

[Kevin Daudt]

On Tue, Aug 22, 2017 at 05:55:25PM -0400, Jeffrey Walton wrote:
> I tested some changes that lead to a dead end. The changes need to be
> removed. The changes were added in 7 commits.
> 
> I went back in time to the point before the changes:
> 
> $ git reset --hard HEAD~7
> HEAD is now at 559fc3b Fix benchmark selection code (GH #464)
> 
> When I attempted to push:
> 
> $ git push
> Username for 'https://github.com': noloader
> To https://github.com/noloader/cryptopp.git
>  ! [rejected]master -> master (non-fast-forward)
> 
> I tried to commit, but Git claims there's nothing to add:
> 
> $ git commit
> On branch master
> Your branch is behind 'origin/master' by 7 commits, and can be
> fast-forwarded.
> 
> Commit seems to be the wrong command as Git appears to be trying to do
> something I don't want.
> 
> How do I force the push to succeed?
> 
> Thanks in advance.

By actually doing a force push:

git push --force-with-lease origin master

But note that this can interfere with others working on the same
repository, so be carefull when you use a force push.

Additionally, I can recommend using separate branches for things that
might turn up as dead ends (or even for every development). That way,
you can just throw away the branch if you want to discard it.

Hope this helps, Kevin.

Re: git fetch with refspec does not include tags?

[Kevin Daudt]

On Thu, Aug 17, 2017 at 01:38:36PM -0700, Junio C Hamano wrote:
> Kevin Daudt <m...@ikke.info> writes:
> 
> > On Thu, Aug 17, 2017 at 12:38:58PM -0700, Junio C Hamano wrote:
> >> Jeff King <p...@peff.net> writes:
> >> 
> >> >   # no tags, we just populate FETCH_HEAD because of the bare URL
> >> >   git fetch ../parent
> >> >
> >> >   # this does fetch tags, because we're storing the result according to
> >> >   # the configured refspec ("refs/heads/*:refs/remotes/origin/*").
> >> >   git fetch origin
> >> 
> >> The above two look good.
> >> 
> >> >   # this doesn't fetch tags, as the main command is "just" populating
> >> >   # FETCH_HEAD. But then our logic for "hey, we fetched the ref for
> >> >   # refs/remotes/origin/master, so let's update it on the side" kicks
> >> >   # in. And we end up updating FETCH_HEAD _and_ the tracking branch, but
> >> >   # not the tags. Weird.
> >> >   git fetch origin master
> >> 
> >> Yes, it looks weird, but I suspect that it is probably more correct
> >> not to fetch tags in this case.  I wonder if it would be a solution
> >> not to do the "on the side" thing---after all the user didn't tell
> >> us to update refs/remotes/origin/master with this command line.
> >
> > Isn't that how git fetch used to behave, or am I misunderstanding what
> > you mean? It used to be that git fetch   would not
> > update any remote tracking branches.
> >
> > From the 1.8.4 release notes:
> >
> >> "git fetch origin master" unlike "git fetch origin" or "git fetch"
> >> did not update "refs/remotes/origin/master"; this was an early
> >> design decision to keep the update of remote tracking branches
> >> predictable, but in practice it turns out that people find it more
> >> convenient to opportunistically update them whenever we have a
> >> chance, and we have been updating them when we run "git push" which
> >> already breaks the original "predictability" anyway.
> 
> No, you are not misunderstanding anything.  The "pretend that we
> immediately turned around and fetched" done by "git push" was
> already breaking the predictability, but the change in 1.8.4 made it
> even worse.  I am saying that going back to the old behaviour may be
> one option to address the issue being discussed in this thread.
> 

Ok. The reason I'm bring this up is because we often had to tell users
in the irc channel that git fetch   did not update the
remote tracking branches, which confused them, so reverting back might
reintroduce this confusion again.

Re: git fetch with refspec does not include tags?

[Kevin Daudt]

On Thu, Aug 17, 2017 at 12:38:58PM -0700, Junio C Hamano wrote:
> Jeff King  writes:
> 
> >   # no tags, we just populate FETCH_HEAD because of the bare URL
> >   git fetch ../parent
> >
> >   # this does fetch tags, because we're storing the result according to
> >   # the configured refspec ("refs/heads/*:refs/remotes/origin/*").
> >   git fetch origin
> 
> The above two look good.
> 
> >   # this doesn't fetch tags, as the main command is "just" populating
> >   # FETCH_HEAD. But then our logic for "hey, we fetched the ref for
> >   # refs/remotes/origin/master, so let's update it on the side" kicks
> >   # in. And we end up updating FETCH_HEAD _and_ the tracking branch, but
> >   # not the tags. Weird.
> >   git fetch origin master
> 
> Yes, it looks weird, but I suspect that it is probably more correct
> not to fetch tags in this case.  I wonder if it would be a solution
> not to do the "on the side" thing---after all the user didn't tell
> us to update refs/remotes/origin/master with this command line.

Isn't that how git fetch used to behave, or am I misunderstanding what
you mean? It used to be that git fetch   would not
update any remote tracking branches.

>From the 1.8.4 release notes:

> "git fetch origin master" unlike "git fetch origin" or "git fetch"
> did not update "refs/remotes/origin/master"; this was an early
> design decision to keep the update of remote tracking branches
> predictable, but in practice it turns out that people find it more
> convenient to opportunistically update them whenever we have a
> chance, and we have been updating them when we run "git push" which
> already breaks the original "predictability" anyway.

Re: git clean -fdx deletes tracked files

[Kevin Daudt]

On Tue, Aug 15, 2017 at 08:45:20PM +0200, Kim Birkelund wrote:
> Hi
> 
> I hope this is gonna sound as weird to you as it does to me.
> 
> The link below is a zip of a small git repository that I can reproduce
> the bug in on 2 machines.
> 
> Repo: https://www.dropbox.com/s/fz4d0i5ko7s7ktr/test.zip?dl=0
> 
> It contains 2 folders: helpers and b, each of which is an empty npm
> module. b\package.json refers to the helpers module.
> 
> The following reproduces the bug:
> 
> 1) in terminal cd to the b folder
> 2) run npm install
> 3) run git reset HEAD --hard
> 4) run git clean -fdx
> 
> At this point both files in the helpers folder has been deleted and
> running git status confirms this.
> 
> Tool version:
> 
> git --version => git version 2.10.2.windows.1
> node -v => v6.11.2
> npm -v => 5.3.0
> 
> 
> I have no idea what is going. Very much hope you can explain :-)

I cannot reproduce it on linux.

git clean -fdx output:

  Removing node_modules/
  Removing package-lock.json

These are all untracked, and nothing in the helpers dir is being
removed.

[PATCH v2] stash: prevent warning about null bytes in input

[Kevin Daudt]

The `no_changes` function calls the `untracked_files` function through
command substitution. `untracked_files` will return null bytes because it
runs ls-files with the '-z' option.

Bash since version 4.4 warns about these null bytes. As they are not
required for the test that is being done, make sure `untracked_files`
does not output null bytes when not required.

This is achieved by adding a parameter to the `untracked_files` function to
specify wither `-z` should be passed to ls-files or not.

This warning is triggered when running git stash save -u resulting in
two warnings:

git-stash: line 43: warning: command substitution: ignored null byte
in input

Signed-off-by: Kevin Daudt <m...@ikke.info>
---
 git-stash.sh | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 9b6c2da7b..5f09a47f0 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -43,9 +43,16 @@ no_changes () {
 }
 
 untracked_files () {
+   if test "$1" = "-z"
+   then
+   shift
+   z=-z
+   else
+   z=
+   fi
excl_opt=--exclude-standard
test "$untracked" = "all" && excl_opt=
-   git ls-files -o -z $excl_opt -- "$@"
+   git ls-files -o $z $excl_opt -- "$@"
 }
 
 clear_stash () {
@@ -114,7 +121,7 @@ create_stash () {
# Untracked files are stored by themselves in a parentless 
commit, for
# ease of unpacking later.
u_commit=$(
-   untracked_files "$@" | (
+   untracked_files -z "$@" | (
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
rm -f "$TMPindex" &&
-- 
2.14.1.145.gb3622a4ee9

Re: [PATCH] stash: prevent warning about null bytes in input

[Kevin Daudt]

On Mon, Aug 14, 2017 at 12:51:26PM -0700, Junio C Hamano wrote:
> Kevin Daudt <m...@ikke.info> writes:
> 
> > The no_changes function calls the untracked_files function through
> > command substitution. untracked_files will return null bytes because it
> > runs ls-files with the '-z' option.
> >
> > Bash since version 4.4 warns about these null bytes. As they are not
> > required for the test that is being done, remove null bytes from the
> > input.
> 
> That's an interesting one ;-)
> 
> I wonder if you considered giving an option to untracked_files
> helper function, though.  After all, it has only two callers,
> and it feels a bit suboptimal to ask the command to do a special
> thing (i.e. "-z") only to clean it up with a pipe.

As a matter of fact, I did not consider that option. I do agree that's a
much better approach.

> 
> IOW, something along the lines of (totally untested)...
> 

How should I proceed with this? Resubmit it after testing with the
appropriate attribution?


>  git-stash.sh | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/git-stash.sh b/git-stash.sh
> index 9b6c2da7b4..5f09a47f0a 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -43,9 +43,16 @@ no_changes () {
>  }
>  
>  untracked_files () {
> + if test "$1" = "-z"
> + then
> + shift
> + z=-z
> + else
> + z=
> + fi
>   excl_opt=--exclude-standard
>   test "$untracked" = "all" && excl_opt=
> - git ls-files -o -z $excl_opt -- "$@"
> + git ls-files -o $z $excl_opt -- "$@"
>  }
>  
>  clear_stash () {
> @@ -114,7 +121,7 @@ create_stash () {
>   # Untracked files are stored by themselves in a parentless 
> commit, for
>   # ease of unpacking later.
>   u_commit=$(
> - untracked_files "$@" | (
> + untracked_files -z "$@" | (
>   GIT_INDEX_FILE="$TMPindex" &&
>   export GIT_INDEX_FILE &&
>   rm -f "$TMPindex" &&
> 
> 

[PATCH] stash: prevent warning about null bytes in input

[Kevin Daudt]

The no_changes function calls the untracked_files function through
command substitution. untracked_files will return null bytes because it
runs ls-files with the '-z' option.

Bash since version 4.4 warns about these null bytes. As they are not
required for the test that is being done, remove null bytes from the
input.

This warning is triggered when running git stash save -u resulting in
two warnings:

git-stash: line 43: warning: command substitution: ignored null byte
in input

Signed-off-by: Kevin Daudt <m...@ikke.info>
---
 git-stash.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-stash.sh b/git-stash.sh
index 9b6c2da7b..0dcca3cd6 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -39,7 +39,7 @@ fi
 no_changes () {
git diff-index --quiet --cached HEAD --ignore-submodules -- "$@" &&
git diff-files --quiet --ignore-submodules -- "$@" &&
-   (test -z "$untracked" || test -z "$(untracked_files)")
+   (test -z "$untracked" || test -z "$(untracked_files | tr -d '\0')")
 }
 
 untracked_files () {
-- 
2.14.0.rc1.33.g384a8b271c

Re: Bug when stashing previously-ignored file plus associated .gitignore change

[Kevin Daudt]

On Fri, Aug 11, 2017 at 04:55:38PM +0100, Sam Partington wrote:
> Hi there,
> 
> I'm running git 2.7.4 on Ubuntu 16.04.  I've found a couple of
> problems when "un-ignoring" files in tandem with git stash.
> 
> Here's how to reproduce:
> 
> Say you have a project using git, with a .gitignore file which
> contains the following line:
> 
> bin/*
> 
> You can then see the problems by doing this:
> 
> $ touch bin/mynewfile # this file will be ignored at this point > 
> and then updating .gitignore to look like this (adding that second line):
> 
> bin/*
> !bin/mynewfile
> 
> So far, so good; the new file is no longer ignored.
> 
> But now, try stashing the changes and including untracked files in the stash:
> 
> $ git stash save -u
> 
> Here's the first problem, bin/mynewfile is still there:
> 
> $ ls bin/mynewfile
> bin/mynewfile
> 
> But you'd expect it to not be there and be in the stash, I think.
> This is what would normally happen with the untracked-files option for
> git stash.
> 
> This leads to the second problem - you can't now pop the stash:
> 
> $ git stash pop
> bin/mynewfile already exists, no checkout
> Could not restore untracked files from stash
> 
> If you want to apply the stash, you have to remove the file:
> 
> $ rm bin/mynewfile
> $ git stash pop # this works, and re-creates bin/mynewfile
> 
> This is quite an unusual edge case, but I have hit it two or three
> times now and so thought it worth reporting, but I'll understand if
> it's deemed not worth fixing!
> 
> Do let me know if you need any more information from me here.
> 
> Thanks
> Sam
> 
> PS Sorry for the lack of formatting - I'm sending this as plain text
> as my original HTML emails was rejected as possible spam by your
> mailserver.
> 
> Sam Partington
> Senior Developer
> 

Hello Sam,

Is it the case that you did not commit the addition of '!bin/mynewfile'
yet? I suspect that by running git stash save -u, you also are stashing
this addition to the .gitigore file. Can you confirm this?

Kevin

Re: Small typo in german translation

[Kevin Daudt]

On Mon, Jul 10, 2017 at 08:47:23AM +0200, Andre Hinrichs wrote:
> Hi Git-Developers!
> 
> I've found a small typo in git/po/de.po
> In line 8567 the word "erwzingen" should be "erzwingen".
> Please fix.
> 
> Thanks
> 

Hello Andre,

The best way to get this fixed is to make a pull-request on the German
translation team repo[0].

Kind regards, Kevin

[0]: https://github.com/ralfth/git-po-de

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

[Kevin Daudt]

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

The function is called "rest_is_empty".

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

Kevin

Re: Small issue with "add untracked" option of 'git add -i'

[Kevin Daudt]

On Wed, Jun 21, 2017 at 08:25:26AM +0530, Kaartic Sivaraam wrote:
> 
> I tried applying the patch and building it locally. For some reason I
> couldn't see the change in effect. What could I be missing?
> 

Did you make sure you used the git you built, and also the relevant
subcommands?

What does `which git` and git --exec-path return?

If you did not install git, but try to run it in the compiled dir, you
can better run bin-wrappers/git, which will make sure the correct sub
commands are run.

Re: 'pu' broken at t5304 tonight

[Kevin Daudt]

On Sat, Jun 10, 2017 at 02:48:36PM +0200, Kevin Daudt wrote:
> On Sat, Jun 10, 2017 at 03:07:01PM +0900, Junio C Hamano wrote:
> > I didn't check where it goes wrong.  Here is a list of suspects,
> > taken by
> > 
> > $ git shortlog --no-merges pu@{8.hours}..pu
> > 
> > i.e. patches that weren't in pu before today's integration cycle.
> > 
> > Andreas Heiduk (1):
> >   doc: describe git svn init --ignore-refs
> > 
> > Brandon Williams (32):
> >   config: create config.h
> >   config: remove git_config_iter
> >   config: don't include config.h by default
> >   config: don't implicitly use gitdir
> >   setup: don't perform lazy initialization of repository state
> >   environment: remove namespace_len variable
> >   repository: introduce the repository object
> >   environment: place key repository state in the_repository
> >   environment: store worktree in the_repository
> >   setup: add comment indicating a hack
> >   config: read config from a repository object
> >   repository: add index_state to struct repo
> >   submodule-config: store the_submodule_cache in the_repository
> >   submodule: add repo_read_gitmodules
> >   submodule: convert is_submodule_initialized to work on a repository
> >   convert: convert get_cached_convert_stats_ascii to take an index
> >   convert: convert crlf_to_git to take an index
> >   convert: convert convert_to_git_filter_fd to take an index
> >   convert: convert convert_to_git to take an index
> >   convert: convert renormalize_buffer to take an index
> >   tree: convert read_tree to take an index parameter
> >   ls-files: convert overlay_tree_on_cache to take an index
> >   ls-files: convert write_eolinfo to take an index
> >   ls-files: convert show_killed_files to take an index
> >   ls-files: convert show_other_files to take an index
> >   ls-files: convert show_ru_info to take an index
> >   ls-files: convert ce_excluded to take an index
> >   ls-files: convert prune_cache to take an index
> >   ls-files: convert show_files to take an index
> >   ls-files: factor out debug info into a function
> >   ls-files: factor out tag calculation
> >   ls-files: use repository object
> > 
> > Jeff King (1):
> >   date: use localtime() for "-local" time formats
> > 
> > Johannes Schindelin (8):
> >   discover_git_directory(): avoid setting invalid git_dir
> >   config: report correct line number upon error
> >   help: use early config when autocorrecting aliases
> >   read_early_config(): optionally return the worktree's top-level 
> > directory
> >   t1308: relax the test verifying that empty alias values are disallowed
> >   t7006: demonstrate a problem with aliases in subdirectories
> >   alias_lookup(): optionally return top-level directory
> >   Use the early config machinery to expand aliases
> > 
> > Junio C Hamano (1):
> >   ### match next
> > 
> > Prathamesh Chavan (1):
> >   dir: create function count_slashes
> > 
> > SZEDER Gábor (5):
> >   revision.h: turn rev_info.early_output back into an unsigned int
> >   revision.c: stricter parsing of '--no-{min,max}-parents'
> >   revision.c: stricter parsing of '--early-output'
> >   revision.c: use skip_prefix() in handle_revision_opt()
> >   revision.c: use skip_prefix() in handle_revision_pseudo_opt()
> > 
> > Stefan Beller (1):
> >   t4005: modernize style and drop hard coded sha1
> > 
> 
> For me, this bisects to the latest merge:
> 
> 2047eebd3 (Merge branch 'bw/repo-object' into pu, 2017-06-10), but
> neither of the parent of the merge break this test, so it looks like
> it's because of an interaction between the repo-object topic and another
> topic.

Merging the repo-object with different other topic branches reveals this
topic to cause the bad interaction:

b56c91004 (Merge branch 'nd/prune-in-worktree' into pu, 2017-06-10)

Still investigating why it happens.

Re: 'pu' broken at t5304 tonight

[Kevin Daudt]

On Sat, Jun 10, 2017 at 03:07:01PM +0900, Junio C Hamano wrote:
> I didn't check where it goes wrong.  Here is a list of suspects,
> taken by
> 
> $ git shortlog --no-merges pu@{8.hours}..pu
> 
> i.e. patches that weren't in pu before today's integration cycle.
> 
> Andreas Heiduk (1):
>   doc: describe git svn init --ignore-refs
> 
> Brandon Williams (32):
>   config: create config.h
>   config: remove git_config_iter
>   config: don't include config.h by default
>   config: don't implicitly use gitdir
>   setup: don't perform lazy initialization of repository state
>   environment: remove namespace_len variable
>   repository: introduce the repository object
>   environment: place key repository state in the_repository
>   environment: store worktree in the_repository
>   setup: add comment indicating a hack
>   config: read config from a repository object
>   repository: add index_state to struct repo
>   submodule-config: store the_submodule_cache in the_repository
>   submodule: add repo_read_gitmodules
>   submodule: convert is_submodule_initialized to work on a repository
>   convert: convert get_cached_convert_stats_ascii to take an index
>   convert: convert crlf_to_git to take an index
>   convert: convert convert_to_git_filter_fd to take an index
>   convert: convert convert_to_git to take an index
>   convert: convert renormalize_buffer to take an index
>   tree: convert read_tree to take an index parameter
>   ls-files: convert overlay_tree_on_cache to take an index
>   ls-files: convert write_eolinfo to take an index
>   ls-files: convert show_killed_files to take an index
>   ls-files: convert show_other_files to take an index
>   ls-files: convert show_ru_info to take an index
>   ls-files: convert ce_excluded to take an index
>   ls-files: convert prune_cache to take an index
>   ls-files: convert show_files to take an index
>   ls-files: factor out debug info into a function
>   ls-files: factor out tag calculation
>   ls-files: use repository object
> 
> Jeff King (1):
>   date: use localtime() for "-local" time formats
> 
> Johannes Schindelin (8):
>   discover_git_directory(): avoid setting invalid git_dir
>   config: report correct line number upon error
>   help: use early config when autocorrecting aliases
>   read_early_config(): optionally return the worktree's top-level 
> directory
>   t1308: relax the test verifying that empty alias values are disallowed
>   t7006: demonstrate a problem with aliases in subdirectories
>   alias_lookup(): optionally return top-level directory
>   Use the early config machinery to expand aliases
> 
> Junio C Hamano (1):
>   ### match next
> 
> Prathamesh Chavan (1):
>   dir: create function count_slashes
> 
> SZEDER Gábor (5):
>   revision.h: turn rev_info.early_output back into an unsigned int
>   revision.c: stricter parsing of '--no-{min,max}-parents'
>   revision.c: stricter parsing of '--early-output'
>   revision.c: use skip_prefix() in handle_revision_opt()
>   revision.c: use skip_prefix() in handle_revision_pseudo_opt()
> 
> Stefan Beller (1):
>   t4005: modernize style and drop hard coded sha1
> 

For me, this bisects to the latest merge:

2047eebd3 (Merge branch 'bw/repo-object' into pu, 2017-06-10), but
neither of the parent of the merge break this test, so it looks like
it's because of an interaction between the repo-object topic and another
topic.

Re: [PATCH] Fourth batch for 2.14

[Kevin Daudt]

On Wed, Jun 07, 2017 at 12:33:41PM +0200, Kevin Daudt wrote:
> From: Junio C Hamano <gits...@pobox.com>
> 

sorry for the noise, please ignore this.

[PATCH] Fourth batch for 2.14

[Kevin Daudt]

From: Junio C Hamano <gits...@pobox.com>

Signed-off-by: Junio C Hamano <gits...@pobox.com>
Signed-off-by: Kevin Daudt <m...@ikke.info>
---
 Documentation/RelNotes/2.14.0.txt | 55 +++
 1 file changed, 55 insertions(+)

diff --git a/Documentation/RelNotes/2.14.0.txt 
b/Documentation/RelNotes/2.14.0.txt
index ee6c6075a..d0f7ef559 100644
--- a/Documentation/RelNotes/2.14.0.txt
+++ b/Documentation/RelNotes/2.14.0.txt
@@ -44,6 +44,22 @@ UI, Workflows & Features
  * "git repack" learned to accept the --threads= option and pass it
to pack-objects.
 
+ * "git send-email" learned to run sendemail-validate hook to inspect
+   and reject a message before sending it out.
+   (merge 6489660b4b jt/send-email-validate-hook later to maint).
+
+ * There is no good reason why "git fetch $there $sha1" should fail
+   when the $sha1 names an object at the tip of an advertised ref,
+   even when the other side hasn't enabled allowTipSHA1InWant.
+
+ * The recently introduced "[includeIf "gitdir:$dir"] path=..."
+   mechansim has further been taught to take symlinks into account.
+   The directory "$dir" specified in "gitdir:$dir" may be a symlink to
+   a real location, not something that $(getcwd) may return.  In such
+   a case, a realpath of "$dir" is compared with the real path of the
+   current repository to determine if the contents from the named path
+   should be included.
+
 
 Performance, Internal Implementation, Development Support etc.
 
@@ -75,6 +91,24 @@ Performance, Internal Implementation, Development Support 
etc.
 
  * Conversion from uchar[20] to struct object_id continues.
 
+ * Simplify parse_pathspec() codepath and stop it from looking at the
+   default in-core index.
+   (merge 08de9151a8 bw/pathspec-sans-the-index later to maint).
+
+ * Add perf-test for wildmatch.
+   (merge 62ca75a6b9 ab/perf-wildmatch later to maint).
+
+ * Code from "conversion using external process" codepath has been
+   extracted to a separate sub-process.[ch] module.
+   (merge 4f2a2e9f0e bp/sub-process-convert-filter later to maint).
+
+ * When "git checkout", "git merge", etc. manipulates the in-core
+   index, various pieces of information in the index extensions are
+   discarded from the original state, as it is usually not the case
+   that they are kept up-to-date and in-sync with the operation on the
+   main index.  The untracked cache extension is copied across these
+   operations now, which would speed up "git status" (as long as the
+   cache is properly invalidated).
 
 
 Also contains various documentation updates and code clean-ups.
@@ -209,6 +243,26 @@ notes for details).
should silently be ignored instead)
(merge a3ba6bf10a jk/ignore-broken-tags-when-ignoring-missing-links later 
to maint).
 
+ * "git describe --contains" penalized light-weight tags so much that
+   they were almost never considered.  Instead, give them about the
+   same chance to be considered as an annotated tag that is the same
+   age as the underlying commit would.
+   (merge ef1e74065c jc/name-rev-lw-tag later to maint).
+
+ * The "run-command" API implementation has been made more robust
+   against dead-locking in a threaded environment.
+   (merge e3f43ce765 bw/forking-and-threading later to maint).
+
+ * A recent update to t5545-push-options.sh started skipping all the
+   tests in the script when a web server testing is disabled or
+   unavailable, not just the ones that require a web server.  Non HTTP
+   tests have been salvaged to always run in this script.
+   (merge 2e397e4ddf jc/skip-test-in-the-middle later to maint).
+
+ * "git send-email" now uses Net::SMTP::SSL, which is obsolete, only
+   when needed.  Recent versions of Net::SMTP can do TLS natively.
+   (merge 0ead000c3a dk/send-email-avoid-net-smtp-ssl-when-able later to 
maint).
+
  * Other minor doc, test and build updates and code cleanups.
(merge 515360f9e9 jn/credential-doc-on-clear later to maint).
(merge 0e6d899fee ab/aix-needs-compat-regex later to maint).
@@ -217,3 +271,4 @@ notes for details).
(merge c8f7c8b704 tb/dedup-crlf-tests later to maint).
(merge 449456ad47 sg/core-filemode-doc-typofix later to maint).
(merge ba4dce784e km/log-showsignature-doc later to maint).
+   (merge c5a9157393 jh/memihash-opt later to maint).
-- 
2.13.0.rc2.2.g9b669787fc

Re: [BUG] Failed to sign commit

[Kevin Daudt]

On Wed, Jun 07, 2017 at 10:46:08AM +0100, pedro rijo wrote:
> Recently I've updated a bunch of stuff, including git and gpg. I'm using
> 
> - mac OS 10.10.5
> - git 2.13.1
> - gpg (GnuPG) 2.1.21 / libgcrypt 1.7.7
> 
> When I do
> 
> $ git commit --allow-empty -v -m "lol"
> error: gpg failed to sign the data
> fatal: failed to write commit object
> 
> I tried the verbose flag hoping to have a better insight, but not very
> useful. Not sure if it's a gpg problem, a git problem, or something
> else.
> 
> Any clue on how to debug the problem? Do you need any gpg output to
> better understand the problem?
> 
> Thanks,
> Pedro

GIT_TRACE=1 git commit --allow-empty -v -m "lol" might give some extra
feedback (ie, what gpg command git runs), and try to see if you can
replicate it.

Re: Git Merge 2017 Videos

[Kevin Daudt]

On Sun, Jun 04, 2017 at 11:24:17AM +0100, Philip Oakley wrote:
> While looking at the recent .gitignore issue (the need to use `**`) I came
> up against a comment in
> https://public-inbox.org/git/cagz79kzqsaubfotjyqm+-+ljyyec2ykj5exuy5kderezfh0...@mail.gmail.com/
> noting that the Git Merge 2017 videos were not available at that time.
> 
> Well, a search found them on Youtube on the GitHub channel :
> https://www.youtube.com/results?search_query=git+merge+2017+videos
> 
> With a playlist : 
> https://www.youtube.com/watch?v=tvymSWfvkjw=PL0lo9MOBetEGRAJzoTCdco_fOKDfhqaOY
> 
> Enjoy the viewing. The first few have been good.
> 

Thanks for sharing this.

Re: wrong language translation part7

[Kevin Daudt]

On Thu, Jun 01, 2017 at 01:16:11PM +0200, SJR wrote:
> W dniu 1 czerwca 2017 09:43 użytkownik SJR  napisał:
> >
> > Hi,
> >
> > https://git-scm.com/book/pl/v1/Dostosowywanie-Gita-Konfiguracja-Gita
> >
> > part in polish part in english.
> >
> > Can You repair translation?
> >
> > Regards,
> > JanR

The progit book is not maintained by the git community itself. You can
find the project at [0].

If you look at this specific chapter in the Polish translation[1], then
you'll see that that chapter is just not completely translated. Someone
would need to step in and finish the translation.

But note that this is version 1 of the book, while the latest release is
version 2. But that one does not have a polish translation at all.

[0]: https://github.com/progit/progit/
[1]: 
https://github.com/progit/progit/blob/master/pl/07-customizing-git/01-chapter7.markdown

Re: git rebase regression: cannot pass a shell expression directly to --exec

[Kevin Daudt]

On Tue, May 16, 2017 at 09:59:03AM -0700, Eric Rannaud wrote:
> On Tue, May 16, 2017 at 9:18 AM, Jeff King  wrote:
> > On Tue, May 16, 2017 at 12:23:02PM +0200, Johannes Schindelin wrote:
> >> It would appear to me that you used a side effect of an implementation
> >> detail: that `git rebase -i` was implemented entirely as a shell script.
> >
> > I don't think that's true at all. He expected the user-provided "--exec"
> > command to be run by a shell, which seems like a reasonable thing for
> > Git to promise (and we already make a similar promise for most
> > user-provided commands that we run).  What happens in between, be it
> 
> As a "user", my expectation was simply that the command would be run
> not just in "a shell", but in *my* shell (or the shell that calls git,
> maybe). So I don't see any portability question with respect to Git.
> My script that uses git rebase --exec may not be portable, but that's
> my problem.
> 
> When I use "git rebase --exec " I'm basically writing a "foreach
> commit in range {  }" in my shell. Same idea with git bisect run.
> 
> A transparent optimization that tries execve() then falls back to the
> user's shell sounds like a good idea.

It does not really work that way. Git runs in a separate process that
does not have access to your current shell. That's why you need to do
'export -f foo'.

If you want git to be able to ecute the foo shell function, git needs to
start a _new_ shell process, which reads the environment, recognize the
exported function and run that.

This is not the same as git executing the command in your shell. Not
exported variables would not be available in this function (as it would
be in your equivalent).

Re: Cant clone/pull/fetch because of "Unable to create temporary file '$HOME/Dev/linux-stable/.git/objects/pack/tmp_pack_'

[Kevin Daudt]

On Mon, May 15, 2017 at 12:28:58AM +0200, Thomas Schweikle wrote:
> $ git clone
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git
> linux-stable
> Cloning into 'linux-stable'...
> remote: Counting objects: 5932092, done.
> remote: Compressing objects: 100% (154131/154131), done.
> fatal: Unable to create temporary file
> '$HOME/Dev/linux-stable/.git/objects/pack/tmp_pack_XX': Permission
> denied
> fatal: index-pack failed
> 
> Since no file/directory created by git I cant tell why git isn't
> able to create
> "$HOME/Dev/linux-stable/.git/objects/pack/tmp_pack_XX".
> 
> If I try to create this file and directory I can create it:
> $ mkdir -p $HOME/Dev/linux-stable/.git/objects/pack
> $ touch $HOME/Dev/linux-stable/.git/objects/pack/tmp_pack_XX
> $ ll $HOME/Dev/linux-stable/.git/objects/pack/tmp_pack_XX
> -rw-rw-r-x+ 1 tps tps 0 May 15 00:18
> /home/tps/Dev/linux-stable/.git/objects/pack/tmp_pack_XX
> $
> 
> $ git --version
> git version 2.11.0
> 
> -- 
> Thomas

Note the '+' at the end of the permission list. This means an acl has
been applied to this directory.

What does getfacl
'$HOME/Dev/linux-stable/.git/objects/pack/' return?

Re: git error

[Kevin Daudt]

On Fri, May 05, 2017 at 10:52:13AM +0800, ada wrote:
> hi,
> 
> I used the command below,there's an error,please help me to solve it !
> Thank you ~
> git push
> 
> 
> Best Regards~
> ada Wang
> 

Hello Ada,

In order for people to be able to help you, you should provide more
details about your problem.

This typically includes what exact command you ran, what the output /
error message you got and any other relevant details.

Kind regards, Kevin.

Re: PCRE v2 compile error, was Re: What's cooking in git.git (May 2017, #01; Mon, 1)

[Kevin Daudt]

On Tue, May 02, 2017 at 08:52:21PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
>  * Due to the bizarro existing semantics of the configure script noted
> upthread if you have a git build script that does --with-libpcre & you
> have libpcre1 installed, it'll link to it, but now since
> --with-libpcre defaults to libpcre2 it'll silently skip linking to it
> if you don't have it installed.
> 

Case in point: The Archlinux git-git aur package[0] (community maintained,
latest git version) does run ./configure without --with-libpcre, but
 requests it from make with USE_LIBPCRE=1.

I noticed when trying git grep -P which then failed.

[0]: https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=git-git