Re: why doesn't "git reset" mention optional pathspec?

[Duy Nguyen]

On Sat, Dec 8, 2018 at 6:37 PM Robert P. J. Day  wrote:
>
> On Sat, 8 Dec 2018, Duy Nguyen wrote:
>
> > On Sat, Dec 8, 2018 at 6:32 PM Robert P. J. Day  
> > wrote:
> > >
> > > On Sat, 8 Dec 2018, Duy Nguyen wrote:
> > >
> > > > On Sat, Dec 8, 2018 at 5:08 PM Robert P. J. Day  
> > > > wrote:
> > > > >
> > > > >
> > > > >   from "man git-reset":
> > > > >
> > > > > SYNOPSIS
> > > > >   git reset [-q] [] [--] ...
> > > > >   git reset (--patch | -p) [] [--] [...]
> > > > >   git reset [--soft | --mixed [-N] | --hard | --merge | --keep] [-q] 
> > > > > []
> > > > >
> > > > > oddly, the third form says nothing about possible "", even
> > > > > though i'm pretty sure they're valid in that third case (at least
> > > > > for "--mixed"). thoughts? is that just an oversight in the man
> > > > > page?
> > > >
> > > > --mixed prints a deprecation warning. I don't think it's worth
> > > > making the synopsis more complicated for that. All other modes
> > > > reject pathspec.
> > >
> > >   i just tested this, and i don't see a deprecation warning.
> >
> > Hmm.. maybe I misread the code. I just tried it
> >
> > $ ./git reset --mixed HEAD foo
> > warning: --mixed with paths is deprecated; use 'git reset -- ' 
> > instead.
>
>   weird ... i just tried this two ways, explicitly specifying
> "--mixed" and also without (which is the default mode, right?), and i
> got the deprecated message with the first but not the second. that
> seems ... odd.

Without --mixed, you're using the first form

git reset [-q] [] [--] ...

which accepts pathspec. If it's not clear, of course patches are welcome.
-- 
Duy

Re: why doesn't "git reset" mention optional pathspec?

[Duy Nguyen]

On Sat, Dec 8, 2018 at 6:32 PM Robert P. J. Day  wrote:
>
> On Sat, 8 Dec 2018, Duy Nguyen wrote:
>
> > On Sat, Dec 8, 2018 at 5:08 PM Robert P. J. Day  
> > wrote:
> > >
> > >
> > >   from "man git-reset":
> > >
> > > SYNOPSIS
> > >   git reset [-q] [] [--] ...
> > >   git reset (--patch | -p) [] [--] [...]
> > >   git reset [--soft | --mixed [-N] | --hard | --merge | --keep] [-q] 
> > > []
> > >
> > > oddly, the third form says nothing about possible "", even
> > > though i'm pretty sure they're valid in that third case (at least
> > > for "--mixed"). thoughts? is that just an oversight in the man
> > > page?
> >
> > --mixed prints a deprecation warning. I don't think it's worth
> > making the synopsis more complicated for that. All other modes
> > reject pathspec.
>
>   i just tested this, and i don't see a deprecation warning.

Hmm.. maybe I misread the code. I just tried it

$ ./git reset --mixed HEAD foo
warning: --mixed with paths is deprecated; use 'git reset -- ' instead.
-- 
Duy

Re: why doesn't "git reset" mention optional pathspec?

[Duy Nguyen]

On Sat, Dec 8, 2018 at 5:08 PM Robert P. J. Day  wrote:
>
>
>   from "man git-reset":
>
> SYNOPSIS
>   git reset [-q] [] [--] ...
>   git reset (--patch | -p) [] [--] [...]
>   git reset [--soft | --mixed [-N] | --hard | --merge | --keep] [-q] 
> []
>
> oddly, the third form says nothing about possible "", even
> though i'm pretty sure they're valid in that third case (at least for
> "--mixed"). thoughts? is that just an oversight in the man page?

--mixed prints a deprecation warning. I don't think it's worth making
the synopsis more complicated for that. All other modes reject
pathspec.
-- 
Duy

Re: [PATCH] mailmap: update brandon williams's email address

[Duy Nguyen]

On Sat, Dec 8, 2018 at 7:09 AM Junio C Hamano  wrote:
> If this were "Jonathan asked Brandon if we want to record an address
> we can reach him in our .mailmap file and sent a patch to add one",

Not sure about Jonathan, but I did.

> then the story is different, and I tend to agree with you that such
> a patch is more or less pointless.  That's not the purpose of the
> mailmap file.

Not directly, but when multiple commands use mailmap to show the
canonical mail addresses, then it kinda is. When I look back at an old
commit message, I may look up the author name and address, which is
mapped.

> Not until git-send-email learns to use that file to rewrite
> To/cc/etc to the "canonical" addresses, anyway ;-)

git-send-email does not have to when I copy/paste the address from
git-log anyway.

> I am not sure if there are people whose "canonical" address to be
> used as the author is not necessarily the best address they want to
> get their e-mails at, though.  If we can be reasonably sure that the
> set of such people is empty, then people can take the above mention
> about send-email as a hint about a low-hanging fruit ;-)
>
> Thanks.
>
>


-- 
Duy

Re: [RFC PATCH] Introduce "precious" file concept

[Duy Nguyen]

On Tue, Nov 27, 2018 at 1:56 PM Jacob Keller  wrote:
> Personally, I would rather err on the side which requires the least
> interaction from users to avoid silently clobbering an ignored file.
>
> Either Duy's solution with a sort of "untracked" reflog, or the
> garbage/trashable notion.

The "untracked reflog" is partially functional now [1] if you want to
have a look. I'm not going to post the series until post-2.20, but if
you do look, I suggest the first patch that lays out the design and a
plumbing command to manage it. Basically you'll do

git backup-log --id=worktree log 

then pick up the version you like with "git backup-log cat" and do
whatever you want with it. High level UI is not there and will be a
topic of discussion.

The precious/trashable/garbage notion can be used to suppress making backups.

[1] https://gitlab.com/pclouds/git/commits/backup-log
-- 
Duy

Re: git, monorepos, and access control

[Duy Nguyen]

On Wed, Dec 5, 2018 at 9:46 PM Derrick Stolee  wrote:
> This directory-level security is not a goal for VFS for Git, and I don't
> see itbecoming a priority as it breaks a number of design decisions we
> made in our object storage and communication models.
>
> The best I can think about when considering Git as an approach would be
> to use submodules for your security-related content, and then have server-
> side security for access to those repos. Of course, submodules are not
> supported in VFS for Git, either.

Another option is builtin per-blob encryption (maybe with just
clean/smudge filter), then access control will be about obtaining the
decryption key (*) and we don't break object storage and
communication. Of course pack delta compression becomes absolutely
useless. But that is perhaps an acceptable trade off.

(*) Git will not cache the key in any shape or form. Whenever it needs
to deflate an encrypted blob, it asks for the key from a separate
daemon. This guy handles all the access control.

> The Gerrit service has _branch_ level security, which is related to the
> reachability questions that a directory security would need. However,
> the problem is quite different. Gerrit does have a lot of experience in
> dealing with submodules, though, so that's probably a good place to
> start.
>
> Thanks,
> -Stolee
-- 
Duy

Re: [PATCH/RFC v3 00/14] Introduce new commands switch-branch and restore-files

[Duy Nguyen]

On Tue, Dec 4, 2018 at 7:31 PM Elijah Newren  wrote:
>
> On Tue, Dec 4, 2018 at 10:22 AM Duy Nguyen  wrote:
> >
> > On Tue, Dec 4, 2018 at 6:45 PM Elijah Newren  wrote:
> > > > > > - Two more fancy features (the "git checkout --index" being the
> > > > > >   default mode and the backup log for accidental overwrites) are of
> > > > > >   course still missing. But they are coming.
> > > > > >
> > > > > > I did not go replace "detached HEAD" with "unnamed branch" (or "no
> > > > > > branch") everywhere because I think a unique term is still good to
> > > > > > refer to this concept. Or maybe "no branch" is good enough. I dunno.
> > > > >
> > > > > I personally like "unnamed branch", but "no branch" would still be
> > > > > better than "detached HEAD".
> > > >
> > > > Haven't really worked on killing the term "detached HEAD" yet. But I
> > > > noticed the other day that git-branch reports
> > > >
> > > > * (HEAD detached from 703266f6e4)
> > > >
> > > > and I didn't know how to rephrase that. I guess "unnamed branch from
> > > > 703266f6e4" is probably good enough but my old-timer brain screams no.
> > >
> > > Perhaps "* (On an unnamed branch, at 703266f6e4)"?
> >
> > This 703266f6e4 is the fork point. Once you start adding more commits
> > on top of this unnamed branch, I find it hard to define it "at"
> > 703266f6e4 anymore. "forked from 703266f6e4" (or even starting/growing
> > from...) is probably clearest but also a bit longer.
>
> It reports the fork point rather than the commit HEAD points to?  Ah,
> I guess I never payed that close of attention before.  I actually
> think "on an unnamed branch" is good enough, but if others gain value
> from the extra info, then I understand the conundrum.  I'm not sure
> what the use or rationale is for the fork point, though, so I feel
> slightly at a loss to try to describe this extra piece of info.

It's probably a corner case. This is a better example

* (HEAD detached at pclouds/backup-log)

It does help see i'm working on top of some branch (or tag)
-- 
Duy

Re: [PATCH/RFC v3 00/14] Introduce new commands switch-branch and restore-files

[Duy Nguyen]

On Tue, Dec 4, 2018 at 6:45 PM Elijah Newren  wrote:
> > > > - Two more fancy features (the "git checkout --index" being the
> > > >   default mode and the backup log for accidental overwrites) are of
> > > >   course still missing. But they are coming.
> > > >
> > > > I did not go replace "detached HEAD" with "unnamed branch" (or "no
> > > > branch") everywhere because I think a unique term is still good to
> > > > refer to this concept. Or maybe "no branch" is good enough. I dunno.
> > >
> > > I personally like "unnamed branch", but "no branch" would still be
> > > better than "detached HEAD".
> >
> > Haven't really worked on killing the term "detached HEAD" yet. But I
> > noticed the other day that git-branch reports
> >
> > * (HEAD detached from 703266f6e4)
> >
> > and I didn't know how to rephrase that. I guess "unnamed branch from
> > 703266f6e4" is probably good enough but my old-timer brain screams no.
>
> Perhaps "* (On an unnamed branch, at 703266f6e4)"?

This 703266f6e4 is the fork point. Once you start adding more commits
on top of this unnamed branch, I find it hard to define it "at"
703266f6e4 anymore. "forked from 703266f6e4" (or even starting/growing
from...) is probably clearest but also a bit longer.
-- 
Duy

Re: [PATCH v3 07/14] checkout: split into switch-branch and restore-files

[Duy Nguyen]

On Tue, Dec 4, 2018 at 6:43 PM Elijah Newren  wrote:
> > > > +--ours::
> > > > +--theirs::
> > > > +   Check out stage #2 ('ours') or #3 ('theirs') for unmerged
> > > > +   paths.
> > > > ++
> > > > +Note that during `git rebase` and `git pull --rebase`, 'ours' and
> > > > +'theirs' may appear swapped; `--ours` gives the version from the
> > > > +branch the changes are rebased onto, while `--theirs` gives the
> > > > +version from the branch that holds your work that is being rebased.
> > > > ++
> > > > +This is because `rebase` is used in a workflow that treats the
> > > > +history at the remote as the shared canonical one, and treats the
> > > > +work done on the branch you are rebasing as the third-party work to
> > > > +be integrated, and you are temporarily assuming the role of the
> > > > +keeper of the canonical history during the rebase.  As the keeper of
> > > > +the canonical history, you need to view the history from the remote
> > > > +as `ours` (i.e. "our shared canonical history"), while what you did
> > > > +on your side branch as `theirs` (i.e. "one contributor's work on top
> > > > +of it").
> > >
> > > Total aside because I'm not sure what you could change here, but man
> > > do I hate this.
> >
> > Uh it's actually documented? I'm always confused by this too. --ours
> > and --theirs at this point are pretty much tied to stage 2 and 3.
> > Nothing I can do about it. But if you could come up with some other
> > option names, then we could make "new ours" to be stage 3 during
> > rebase, for example.
>
> I don't think it's a naming issue, personally.  Years ago we could
> have defined --ours and --theirs differently based on which kind of
> operation we were in the middle of, but you are probably right that
> they are now tied to stage 2 and 3.  But there's another place that we
> might still be able to address this; I think the brain-damage here may
> have just been due to the fact that the recursive merge machinery was
> rather inflexible and required HEAD to be stage 2.  If it were a
> little more flexible, then we might just be able to make this problem
> go away.  Maybe it can still be fixed (I haven't dug too deeply into
> it), but if so, the only fix needed here would be to remove this long
> explanation about why the tool gets things totally backward.

Aha. I' not really deep in this merge business to know if stages 2 and
3 can be swapped. This is right up your alley. I'll just leave it to
you.

> > > > +'git switch-branch' -c|-C  []::
> > > > +
> > > > +   Specifying `-c` causes a new branch to be created as if
> > > > +   linkgit:git-branch[1] were called and then switched to. In
> > > > +   this case you can use the `--track` or `--no-track` options,
> > > > +   which will be passed to 'git branch'.  As a convenience,
> > > > +   `--track` without `-c` implies branch creation; see the
> > > > +   description of `--track` below.
> > >
> > > Can we get rid of --track/--no-track and just provide a flag (which
> > > takes no arguments) for the user to use?  Problems with --track:
> > >   * it's not even in your synopsis
> > >   * user has to repeat themselves (e.g. 'next' in two places from '-c
> > > next --track origin/next'); this repetition is BOTH laborious AND
> > > error-prone
> > >   * it's rather inconsistent: --track is the default due to
> > > auto-vivify when the user specifies nothing but a branch name that
> > > doesn't exist yet, but when the user realizes the branch doesn't exist
> > > yet and asks to have it created then suddenly tracking is not the
> > > default??
> >
> > I don't think --track is default anymore (maybe I haven't updated the
> > man page correctly). The dwim behavior is only activated in
> > switch-branch when you specify --guess to reduce the amount of magic
> > we throw at the user. With that in mind, do we still hide
> > --track/--no-track from switch-branch?
>
> Ooh, you're adding --guess?  Cool, that addresses my concerns, just in
> a different manner.

No it's always there even in git-checkout, just hidden.

> Personally, I'd leave --track/--no-track out.  It's extra mental
> overhead, git branch has options for setting those if they need some
> special non-default setup, and if there is enough demand for it we can
> add it later.  Removing options once published is much harder.

Slightly less convenient (you would need a combination of git-branch
and git-switch-branch, if you avoid git-checkout). But since I don't
think I have ever used this option, I'm fine with leaving it out and
considering adding it back later.

> > > I'm not sure what's best, but here's some food for thought:
> > >
> > >
> > >git switch-branch 
> > > switches to , if it exists.  Error cases:
> > >   * If  isn't actually a branch but a  or
> > >  or , error out and suggest using
> > > --detach.
> > >   * If  isn't actually a branch but there is a similarly named
> > >  (e.g. origin/), then suggest using
> > > -c.
> >
> > I would make 

Re: [PATCH/RFC v3 00/14] Introduce new commands switch-branch and restore-files

[Duy Nguyen]

On Tue, Dec 4, 2018 at 2:29 AM Elijah Newren  wrote:
>
> On Thu, Nov 29, 2018 at 2:01 PM Nguyễn Thái Ngọc Duy  
> wrote:
> >
> > v3 sees switch-branch go back to switch-branch (in v2 it was
> > checkout-branch). checkout-files is also renamed restore-files (v1 was
> > restore-paths). Hopefully we won't see another rename.
>
> I started reading through the patches.  I also tried to apply them
> locally, but they had conflicts or missing base file version on both
> master and next.  What version did you base it on?

I think nd/checkout-dwim-fix because of a non-trivial conflict there
(but I don't remember when I noticed it and rebased on that). Anyway
you can get the whole series at

https://gitlab.com/pclouds/git/tree/switch-branch-and-checkout-files

It fixes some of your comments already, a couple of bug fixes here and
there and in a good-enough shape that I start actually using it.

> > - Two more fancy features (the "git checkout --index" being the
> >   default mode and the backup log for accidental overwrites) are of
> >   course still missing. But they are coming.
> >
> > I did not go replace "detached HEAD" with "unnamed branch" (or "no
> > branch") everywhere because I think a unique term is still good to
> > refer to this concept. Or maybe "no branch" is good enough. I dunno.
>
> I personally like "unnamed branch", but "no branch" would still be
> better than "detached HEAD".

Haven't really worked on killing the term "detached HEAD" yet. But I
noticed the other day that git-branch reports

* (HEAD detached from 703266f6e4)

and I didn't know how to rephrase that. I guess "unnamed branch from
703266f6e4" is probably good enough but my old-timer brain screams no.
-- 
Duy

Re: [PATCH v3 07/14] checkout: split into switch-branch and restore-files

[Duy Nguyen]

Thanks for all the comments! There are still some I haven't replied
(either I'll agree and do it anyway, or I'll need some more time to
digest)

On Tue, Dec 4, 2018 at 1:45 AM Elijah Newren  wrote:
> > +'git restore-files' [--from=] ...
>
> Isn't this already inferred by the previous line?  Or was the
> inclusion of --from on the previous line in error?  Looking at the
> git-checkout manpage, it looks like you may have just been copying an
> existing weirdness, but it needs to be fixed.  ;-)

Hehe.

> > +'git restore-files' (-p|--patch) [--from=] [...]
> > +
> > +DESCRIPTION
> > +---
> > +Updates files in the working tree to match the version in the index
> > +or the specified tree.
> > +
> > +'git restore-files' [--from=] ...::
>
>  and ?  I understand  and ,
> or  but have no clue why it'd be okay to specify 
> and  together.  What does that even mean?
>
> Also, rather than fixing from  to  or ,
> perhaps we should just use  here?  (I'm thinking of git
> rev-parse's "Specifying revisions", which suggests "revisions" as a
> good substitute for "commit-ish" that isn't quite so weird for new
> users.)

tree-ish is technically more accurate. But I'm ok with just
. If you give it a blob oid then you should get a nice
explanation what you're doing wrong anyway.


> > +   Overwrite paths in the working tree by replacing with the
> > +   contents in the index or in the  (most often a
> > +   commit).  When a  is given, the paths that
> > +   match the  are updated both in the index and in
> > +   the working tree.
>
> Is that the default we really want for this command?  Why do we
> automatically assume these files are ready for commit?  I understand
> that it's what checkout did, but I'd find it more natural to only
> affect the working tree by default.  We can give it an option for
> affecting the index instead (or perhaps in addition).

Yeah, that behavior of updating the index always bothers me when I use
it but I seemed to forget when working on this.

> > +--ours::
> > +--theirs::
> > +   Check out stage #2 ('ours') or #3 ('theirs') for unmerged
> > +   paths.
> > ++
> > +Note that during `git rebase` and `git pull --rebase`, 'ours' and
> > +'theirs' may appear swapped; `--ours` gives the version from the
> > +branch the changes are rebased onto, while `--theirs` gives the
> > +version from the branch that holds your work that is being rebased.
> > ++
> > +This is because `rebase` is used in a workflow that treats the
> > +history at the remote as the shared canonical one, and treats the
> > +work done on the branch you are rebasing as the third-party work to
> > +be integrated, and you are temporarily assuming the role of the
> > +keeper of the canonical history during the rebase.  As the keeper of
> > +the canonical history, you need to view the history from the remote
> > +as `ours` (i.e. "our shared canonical history"), while what you did
> > +on your side branch as `theirs` (i.e. "one contributor's work on top
> > +of it").
>
> Total aside because I'm not sure what you could change here, but man
> do I hate this.

Uh it's actually documented? I'm always confused by this too. --ours
and --theirs at this point are pretty much tied to stage 2 and 3.
Nothing I can do about it. But if you could come up with some other
option names, then we could make "new ours" to be stage 3 during
rebase, for example.

> > +Part of the linkgit:git[1] suite
>
>
> My single biggest worry about this whole series is that I'm worried
> you're perpetuating and even further ingraining one of the biggest
> usability problems with checkout: people suggest and use it for
> reverting/restoring paths to a previous version, but it doesn't do
> that:
>
> git restore-files --from master~10 Documentation/
> 
> git add -u
> git commit -m "Rationale for changing files including reverting 
> Documentation/"
>
> In particular, now you have a mixture of files in Documentation/ from
> master~10 (er, now likely master~11) and HEAD~1; any files and
> sub-directories that existed in HEAD~1 still remain and are mixed with
> all other files in Documentation/ from the older commit.
>
> You may think this is a special case, but this particular issue
> results in some pretty bad surprises.  Also, it was pretty surprising
> to me just how difficult it was to implement an svn-like revert in
> EasyGit, in large part because of this 'oversight' in git.  git
> checkout --  to me has always been fundamentally wrong, but I
> just wasn't sure if I wanted to fight the backward compatibility
> battle and suggest changing it.  With a new command, we definitely
> shouldn't be reinforcing this error.  (And maybe we should consider
> taking the time to fix git checkout too.)

What would be the right behavior for

 git restore-files --from=master~10 Documentation/

then? Consider it an error? I often use "git checkout HEAD" and "git
checkout HEAD^" (usually with -p) but not very far back like
master~10.

> > +If the branch exists 

Re: [PATCH] rebase -i: introduce the 'test' command

[Duy Nguyen]

On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote:
> I sometimes add "x false" to the top of the todo list to stop and create
> new commits before the first one.

And here I've been doing the same by "edit" the first commit, add a
new commit then reorder them in the second interactive rebase :P

This made me look at git-rebase.txt to really learn about interactive
rebase. I think the interactive rebase section could use some
improvements. Its style looks.. umm.. more story telling than a
reference. Perhaps something like this to at least highlight the
commands.

-- 8< --
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 80793bad8d..c569b3370b 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -637,22 +637,22 @@ The oneline descriptions are purely for your pleasure; 
'git rebase' will
 not look at them but at the commit names ("deadbee" and "fa1afe1" in this
 example), so do not delete or edit the names.
 
-By replacing the command "pick" with the command "edit", you can tell
+By replacing the command `pick` with the command `edit`, you can tell
 'git rebase' to stop after applying that commit, so that you can edit
 the files and/or the commit message, amend the commit, and continue
 rebasing.
 
 To interrupt the rebase (just like an "edit" command would do, but without
-cherry-picking any commit first), use the "break" command.
+cherry-picking any commit first), use the `break` command.
 
 If you just want to edit the commit message for a commit, replace the
-command "pick" with the command "reword".
+command "pick" with the command `reword`.
 
-To drop a commit, replace the command "pick" with "drop", or just
+To drop a commit, replace the command "pick" with `drop`, or just
 delete the matching line.
 
 If you want to fold two or more commits into one, replace the command
-"pick" for the second and subsequent commits with "squash" or "fixup".
+"pick" for the second and subsequent commits with `squash` or `fixup`.
 If the commits had different authors, the folded commit will be
 attributed to the author of the first commit.  The suggested commit
 message for the folded commit is the concatenation of the commit
@@ -693,7 +693,7 @@ $ git rebase -i -p --onto Q O
 Reordering and editing commits usually creates untested intermediate
 steps.  You may want to check that your history editing did not break
 anything by running a test, or at least recompiling at intermediate
-points in history by using the "exec" command (shortcut "x").  You may
+points in history by using the `exec` command (shortcut `x`).  You may
 do so by creating a todo list like this one:
 
 ---
-- 8< --
--
Duy

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

[Duy Nguyen]

On Sun, Dec 2, 2018 at 6:05 PM Robert P. J. Day  wrote:
> >   Patch update>> 2
> >  staged unstaged path
> >   * 1:unchanged+1/-0 README.md
> >   * 2:unchanged+1/-0 contrib/README
> > 3:unchanged+1/-0 t/README
> >   Patch update>>
> >
> > Here I hit enter.  Did you?
>
>   perhaps i'm just not seeing it, but from "man git-add", it doesn't
> seem obvious that you would first select the files to work with, then
> hit a simple CR to get into actual patch mode.

I think it's the same procedure as the "update" step, which describes
this in more detail. I agree that the "patch" section does not make
this obvious.
-- 
Duy

Re: Confusing inconsistent option syntax

[Duy Nguyen]

On Sun, Dec 2, 2018 at 11:13 AM Robert White  wrote:
>
> `git log --pretty short` gives the error message "ambiguous argument
> 'short'". To get the expected result, you need to use `git log
> --pretty=short`. However, `git log --since yesterday` and `git log
> --since=yesterday` both work as expected.
>
> When is an = needed? What is the reason for these inconsistencies?

--pretty can take no arguments. --pretty alone is the same as
--pretty=medium. --since always needs an argument.
-- 
Duy

Re: [RFC PATCH] Introduce "precious" file concept

[Duy Nguyen]

On Wed, Nov 28, 2018 at 10:54 PM Ævar Arnfjörð Bjarmason
 wrote:
> But we must have some viable way to repair warts in the tools, and
> losing user data is a *big* wart.
>
> I don't think something like the endgame you've described in
> https://public-inbox.org/git/xmqqzhtwuhpc@gitster-ct.c.googlers.com/
> is ever going to work. Novice git users (the vast majority) are not
> going to diligently update both .gitignore and some .gitattribute
> mechanism in lockstep. I'd bet most git users haven't read more than a
> few paragraphs of our entire documentation at best.
>
> So what's the way forward? I think ultimately we must move to something
> where we effectively version the entire CLI UI similar to stable API
> versions. I.e. for things like this that would break some things (or
> Duy's new "split checkout") introduce them as flags first, then bundle
> up all such flags and cut a major release "Git 3, 4, ...", and
> eventually remove old functionality.

Related to Duy's new "split chekckout", I just realized that I added
--overwrite-ignore (enabled by default) [1] years ago to allow to out
out of this behavior. We could turn --no-overwrite-ignore by default
on the new command "git switch-branch" to err on the safe side. Then
the user could switch to --overwrite-ignore once they learn more about
gitignore and gitattributes (or the coming "backup log"). I'm not sure
if I really like this, but at least it's one of the options.

[1] 
https://public-inbox.org/git/1322388933-6284-2-git-send-email-pclo...@gmail.com/
-- 
Duy

Re: [PATCH/RFC v3 00/14] Introduce new commands switch-branch and restore-files

[Duy Nguyen]

On Fri, Nov 30, 2018 at 12:29 PM Ævar Arnfjörð Bjarmason
 wrote:
>
>
> On Fri, Nov 30 2018, Duy Nguyen wrote:
>
> > On Fri, Nov 30, 2018 at 12:05 AM Ævar Arnfjörð Bjarmason
> >  wrote:
> >> Assuming greenfield development (which we definitely don't have), I
> >> don't like the "restore-files" name, but the alternative that makes
> >> sense is "checkout". Then this "--from" argument could become "git
> >> checkout-tree  -- ", and we'd have:
> >>
> >> git switch 
> >> git checkout 
> >> git checkout-tree  -- 
> >>
> >> Or maybe that sucks, anyway what I was going to suggest is *if* others
> >> think that made sense as a "if we designed git today" endgame whether we
> >> could have an opt-in setting where you set e.g. core.uiVersion=3 (in
> >> anticipation of Git 3.0) and you'd get that behavior. There could be
> >> some other setting where core.uiVersion would use the old behavior (or
> >> with another setting, only warn) if we weren't connected to a terminal.
> >
> > core.uiVersion is a big no no to me. I don't want to go to someone's
> > terminal, type something and have a total surprise because they set
> > different ui version. If you want a total UI redesign, go with a new
> > prefix, like "ng" (for new git) or something instead of "git".
>
> I don't think that's a viable way forward. First, we're not talking
> about a total change of the UI. Most the main porcelain will stay the
> same. So we'd have a "ng" that's >98% the same UI, and then if we
> discover warts in in 10 years we'd like to fix then what do wo do? Ship
> "nng" and so forth?

Yes. So think it through and try not to do it often.

> We already have this UI problem with various config variables that
> change things. I think we should just solve this general issue by a
> combination of:
>
>  a) Accepting that over the long term we will have Git's UI changing,
> sometimes in breaking ways (with sensible phase-in), hopefully for
> the better. E.g. we had this with "git-init" v.s. "git init". This
> is similar, there'd be an error due to ambiguity with a "hint"
> saying use the new thing if you e.g. feed "git checkout" a branch
> name.

And I hate adding a zillion of config keys to change little things
like this. Now you use this to change bigger behavior. No.
-- 
Duy

Re: [PATCH/RFC v3 00/14] Introduce new commands switch-branch and restore-files

[Duy Nguyen]

On Fri, Nov 30, 2018 at 1:16 AM Dan Fabulich  wrote:
>
> Other thoughts on a global UI rethink:
>
> One of the most common complaints I hear about git is the conceptual 
> difficulty required in undoing changes. https://ohshitgit.com/
>
> > Git is hard: screwing up is easy, and figuring out how to fix your mistakes 
> > is fucking impossible. Git documentation has this chicken and egg problem 
> > where you can't search for how to get yourself out of a mess, unless you 
> > *already know the name of the thing you need to know about* in order to fix 
> > your problem.
>
> A significant fraction of the top-voted questions on StackOverflow are about 
> undoing changes. https://stackoverflow.com/questions/tagged/git
>
> What if there were a 'git undo' command that could unify the answers to all 
> of these questions?
>
> git undo stage
> git undo rm
> git undo edit (checkout files from the stage)
>
> git undo commit (prompt the user whether to revert or reset)
> git undo reset
> git undo checkout
>
> git undo merge
> git undo pull
> git undo push (prompt the user whether to force push back to the past or 
> whether to revert pushed commits)
> git undo rebase
>
> git undo undo
>
> git undo clean
> git undo delete-branch
> git undo delete-stash
>
> Some of these would be trivial effort, but a lot of them would require 
> fundamental changes in the way git operates. (You can't undo a clean right 
> now because the files are just destroyed.)

We're getting there. The biggest problem I have is how this "git undo"
should work, not the changes behind to support it. For example, if I
pulled then did some rebase, what would "git undo pull" do?
-- 
Duy

Re: [PATCH/RFC v3 00/14] Introduce new commands switch-branch and restore-files

[Duy Nguyen]

On Fri, Nov 30, 2018 at 3:16 AM Junio C Hamano  wrote:
>
> Nguyễn Thái Ngọc Duy   writes:
>
> > 'git switch-branch'
> >
> > - implicit detaching is rejected. If you need to detach, you need to
> >   give --detach. Or stick to 'git checkout'.
>
> OK.  Is "auto-vivify the named branch based on a remote-tracking"
> also rejected, as it is a confusing behaviour that is a too subtle
> and implicit, just like the detaching head is, and require --guess
> or sticking to 'git checkout'?  I think it should.

This touches the "remote" concept which I think is another confusing
thing for new people (your "master" is not the same as the server's
"master", aka origin/master) and perhaps this dwim thing helps.
Frankly I don't do dwim much so I don't know if it's that often used.

> > - -b/-B is renamed to -c/-C with long option names
>
> I did not expect that these two are the only options that would be
> out of place with the command name split, but presumably you looked
> at all options for both of the two new commands to see if they made
> sense in the new context?

Yeah (at least the description in struct option[] array)
--
Duy

Re: [PATCH/RFC v3 00/14] Introduce new commands switch-branch and restore-files

[Duy Nguyen]

On Fri, Nov 30, 2018 at 12:05 AM Ævar Arnfjörð Bjarmason
 wrote:
> Assuming greenfield development (which we definitely don't have), I
> don't like the "restore-files" name, but the alternative that makes
> sense is "checkout". Then this "--from" argument could become "git
> checkout-tree  -- ", and we'd have:
>
> git switch 
> git checkout 
> git checkout-tree  -- 
>
> Or maybe that sucks, anyway what I was going to suggest is *if* others
> think that made sense as a "if we designed git today" endgame whether we
> could have an opt-in setting where you set e.g. core.uiVersion=3 (in
> anticipation of Git 3.0) and you'd get that behavior. There could be
> some other setting where core.uiVersion would use the old behavior (or
> with another setting, only warn) if we weren't connected to a terminal.

core.uiVersion is a big no no to me. I don't want to go to someone's
terminal, type something and have a total surprise because they set
different ui version. If you want a total UI redesign, go with a new
prefix, like "ng" (for new git) or something instead of "git".
-- 
Duy

Re: [PATCH v2 6/7] checkout: split into switch-branch and checkout-files

[Duy Nguyen]

On Thu, Nov 29, 2018 at 7:14 PM Stefan Beller  wrote:
>
> > > Idea:
> > > If git checkout-files modifies the submodules file, it could also
> > > auto-update the submodules. (For example, with something like "git
> > > submodule update --init --recursive --progress").
> >
> > This one is tricky because we should deal with submodule autoupdate
> > consistently across all porcelain commands (or at least common ones),
> > not just checkout-files. I'd prefer to delay this until later. Once we
> > figure out what to do with other commands, then we can still change
> > defaults for checkout-files.
>
> checkout/reset are respecting the submodule.recurse setting for this
> already, and as your patches only change the UX frontend
>
> git -c submodule.recurse checkout-files 
>
> would also touch submodules. Given that deep down in
> the submodules it's all about files again, we could think
> checkout-files is still a good name.
>
> I think Stefan X. is asking for making submodule.recurse
> to default to true, which is indeed unrelated to this.

Yes and I'm concerned that checkout-files now recurses into submodules
this by default but grep for example does not. That just adds more
confusion.
-- 
Duy

Re: [PATCH v2 6/7] checkout: split into switch-branch and checkout-files

[Duy Nguyen]

On Thu, Nov 29, 2018 at 12:22 AM Stefan Xenos  wrote:
> Some behaviors I'd expect to see from these commands (I haven't yet
> checked to see if you've already done this):
>
> git checkout-files 
> should reset all the files in the repository regardless of the current
> directory - it should produce the same effect as "git reset --hard
>  && git reset HEAD@{1}". It should also delete
> locally-created files that aren't present in , such that the
> final working tree is exactly identical to what was committed in that
> tree-ish.
>
> git checkout-files foo -- myfile.txt
> should delete myfile.txt if it is present locally but not present in foo.
>
> git checkout-files foo -- .
> should recursively checkout all files in the current folder and all
> subfolders, and delete any locally-created files if they're not
> present in foo.

I think all these are the same as the non-overlay mode Thomas
mentioned [1]. Once he implements that in git-checkout, we can make it
default in checkout-files.

Two things though. I plan to get rid of "--" in checkout-files. The
main use case (I think) is reset from index, so you can just write

 git checkout-files somefiles

and if you want to get it from the tree(-ish) "foo", you do

 git checkout-files --from=foo somefiles

This form is easier to read (and even guess before you read man pages)
and leaves no room for ambiguation.

The second thing is, I plan to forbid "git checkout-files" without
arguments. If you want to reset the entire worktree, do

 git checkout-files :/

or just current dir

 git checkout-files .

Which brings us back to your "git checkout-files " use case
above. It should be treat the same way in my opinion, so we either do

 git checkout-files --from=tree-ish :/

or

 git checkout-files --from=tree-ish .

But "git checkout-files --from=tree-ish" alone is rejected.


[1] 
https://public-inbox.org/git/xmqqwoowo1fk@gitster-ct.c.googlers.com/T/#mdb076d178ccf0ae3dba0fd63143f99278047da93

> Suggestion:
> If git checkout-files overwrites or deletes any locally-modified files
> from the workspace or index, those files could be auto-stashed. That
> would make it easy to restore them in the event of a mistyped command.
> Auto-stashing could be suppressed with a command-line argument (with
> alternate behaviors being fail-if-modified or always-overwrite).

Stashing I think is not the right tool for this. When you stash, you
plan to retrieve it back later but here you should rarely ever need to
unstash until the accident. For recovery from accidents like this, I
have another thing in queue to achieve the same (I'm calling it
"backup log" now). So we will have the same functionality, just with
different means.

> Idea:
> If git checkout-files modifies the submodules file, it could also
> auto-update the submodules. (For example, with something like "git
> submodule update --init --recursive --progress").

This one is tricky because we should deal with submodule autoupdate
consistently across all porcelain commands (or at least common ones),
not just checkout-files. I'd prefer to delay this until later. Once we
figure out what to do with other commands, then we can still change
defaults for checkout-files.
-- 
Duy

Re: [PATCH v2 6/7] checkout: split into switch-branch and checkout-files

[Duy Nguyen]

On Thu, Nov 29, 2018 at 6:59 AM Junio C Hamano  wrote:
>
> Stefan Xenos  writes:
>
> > Although I have no problem with "switch-branch" as a command name,
> > some alternative names we might consider for switch-branch might be:
> >
> > chbranch
> > swbranch
>
> Please never go in that direction.  So far, we made a conscious
> effort to keep the names of most frequently used subcommand to
> proper words that can be understood by coders (IOW, I expect they
> know what 'grep' is, even though that may not be a 'proper word').
>
> > switch
> > branch change (as a subcommand for the "branch" command)
>
> It is more like moving to the branch to work on.  I think 'switch'
> is something SVN veterans may find familiar.  Both are much better
> than ch/swbranch that are overly cryptic.

OK I'll go with switch-branch and restore-files in the next round. And
perhaps consider just 'switch' and 'restore' later.
-- 
Duy

Re: [PATCH/RFC v2 0/7] Introduce new commands switch-branch and checkout-files

[Duy Nguyen]

On Wed, Nov 28, 2018 at 9:30 PM Stefan Beller  wrote:
>
> On Wed, Nov 28, 2018 at 12:09 PM Duy Nguyen  wrote:
> >
> > On Wed, Nov 28, 2018 at 9:01 PM Duy Nguyen  wrote:
> > > should we do
> > > something about detached HEAD in this switch-branch command (or
> > > whatever its name will be)?
> > >
> > > This is usually a confusing concept to new users
> >
> > And it just occurred to me that perhaps we should call this "unnamed
> > branch" (at least at high UI level) instead of detached HEAD. It is
> > technically not as accurate, but much better to understand.
>
> or 'direct' branch?

makes me think, what is an indirect branch?

> I mean 'detached HEAD' itself is also not correct
> as the HEAD points to a valid commit/tag usually, so it is attached to
> that content. The detachment comes from the implicit "from a branch".

Yeah I guess it's short for "HEAD that is detached from a branch"
-- 
Duy

Re: [PATCH/RFC v2 0/7] Introduce new commands switch-branch and checkout-files

[Duy Nguyen]

On Wed, Nov 28, 2018 at 9:01 PM Duy Nguyen  wrote:
> should we do
> something about detached HEAD in this switch-branch command (or
> whatever its name will be)?
>
> This is usually a confusing concept to new users

And it just occurred to me that perhaps we should call this "unnamed
branch" (at least at high UI level) instead of detached HEAD. It is
technically not as accurate, but much better to understand.
-- 
Duy

Re: [PATCH/RFC v2 0/7] Introduce new commands switch-branch and checkout-files

[Duy Nguyen]

On Tue, Nov 27, 2018 at 5:53 PM Nguyễn Thái Ngọc Duy  wrote:
>
> v2 is just a bit better to look at than v1. This is by no means final.
> If you think the command name is bad, the default behavior should
> change, or something else, speak up. It's still very "RFC".
>
> v2 breaks down the giant patch in v1 and starts adding some changes in
> these new commands:
>
> - restore-paths is renamed to checkout-paths. I wrote I didn't like
>   "checkout" because of completion conflict. But who am I kidding,
>   I'll use aliases anyway. "-files" instead of "-paths" because we
>   already have ls-files.
> - both commands will not accept no arguments. There is no "git
>   checkout" equivalent.
> - ambiguation rules are now aware that "switch-branch" for example
>   can't take pathspec...

Another thing. Since we start with a clean slate, should we do
something about detached HEAD in this switch-branch command (or
whatever its name will be)?

This is usually a confusing concept to new users and I've seen some
users have a hard time getting out of it. I'm too comfortable with
detached HEAD to see this from new user's perspective and a better way
to deal with it.
-- 
Duy

Re: [PATCH v2 6/7] checkout: split into switch-branch and checkout-files

[Duy Nguyen]

On Wed, Nov 28, 2018 at 8:08 PM Stefan Beller  wrote:
>
> On Wed, Nov 28, 2018 at 7:31 AM Duy Nguyen  wrote:
> >
> > On Wed, Nov 28, 2018 at 7:03 AM Junio C Hamano  wrote:
> > >
> > > Nguyễn Thái Ngọc Duy   writes:
> > >
> > > > The good old "git checkout" command is still here and will be until
> > > > all (or most of users) are sick of it.
> > >
> > > Two comments on the goal (the implementation looked reasonable
> > > assuming the reader agrees with the gaol).
> > >
> > > At least to me, the verb "switch" needs two things to switch
> > > between, i.e. "switch A and B", unless it is "switch to X".
> > > Either "switch-to-branch" or simply "switch-to", perhaps?
> > >
> > > As I already hinted in my response to Stefan (?) about
> > > checkout-from-tree vs checkout-from-index, a command with multiple
> > > modes of operation is not confusing to people with the right mental
> > > model, and I suspect that having two separate commands for "checking
> > > out a branch" and "checking out paths" that is done by this step
> > > would help users to form the right mental model.
> >
> > Since the other one is already "checkout-files", maybe this one could
> > just be "checkout-branch".
>
> I dislike the checkout-* names, as we already have checkout-index
> as plumbing, so it would be confusing as to which checkout-* command
> should be used when and why as it seems the co-index moves
> content *from index* to the working tree, but the co-files moves content
> *to files*, whereas checkout-branch is neither 'moving' to or from a branch
> but rather 'switching' to that branch.

OK back to square one. Another thing I noticed, not sure if it
matters, is that these two commands will be the only ones with a
hyphen in them in "git help". But I guess it's even harder to find
one-word command names for these.
-- 
Duy

Re: [PATCH v3 1/7] rebase: fix incompatible options error message

[Duy Nguyen]

On Thu, Nov 22, 2018 at 7:32 PM Elijah Newren  wrote:
>
> In commit f57696802c30 ("rebase: really just passthru the `git am`
> options", 2018-11-14), the handling of `git am` options was simplified
> dramatically (and an option parsing bug was fixed), but it introduced
> a small regression in the error message shown when options only
> understood by separate backends were used:
>
> $ git rebase --keep --ignore-whitespace
> fatal: error: cannot combine interactive options (--interactive, --exec,
> --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with
> am options (.git/rebase-apply/applying)
>
> $ git rebase --merge --ignore-whitespace
> fatal: error: cannot combine merge options (--merge, --strategy,
> --strategy-option) with am options (.git/rebase-apply/applying)
>
> Note that in both cases, the list of "am options" is
> ".git/rebase-apply/applying", which makes no sense.  Since the lists of
> backend-specific options is documented pretty thoroughly in the rebase
> man page (in the "Incompatible Options" section, with multiple links
> throughout the document), and since I expect this list to change over
> time, just simplify the error message.

Can we simplify it further and remove the "error: " prefix? "fatal:
error: " looks redundant.
-- 
Duy

Re: [PATCH v2 7/7] Suggest other commands instead of "git checkout"

[Duy Nguyen]

On Wed, Nov 28, 2018 at 7:04 AM Junio C Hamano  wrote:
>
> Nguyễn Thái Ngọc Duy   writes:
>
> > The assumption made is here
> >
> > - "git checkout" is a horrible monster that should only be touched
> >   with a two-meter pole
> >
> > - there are other commands that can achieve the same thing
>
> Thanks for clearly spelling out the assumptions.  It is good that
> this step cames at the end, as the earlier 6 steps looked reasonable
> to me.

I see my deliberate attempt to provoke has failed :D Giving your view
of the new commands as "training wheels", I take it we still should
make them visible as much as possible, but we just not try to hide
"git checkout" as much (e.g. we mention both new and old commands,
instead of just mentioning the new one, when suggesting something)?
-- 
Duy

Re: [PATCH v2 6/7] checkout: split into switch-branch and checkout-files

[Duy Nguyen]

On Wed, Nov 28, 2018 at 7:03 AM Junio C Hamano  wrote:
>
> Nguyễn Thái Ngọc Duy   writes:
>
> > The good old "git checkout" command is still here and will be until
> > all (or most of users) are sick of it.
>
> Two comments on the goal (the implementation looked reasonable
> assuming the reader agrees with the gaol).
>
> At least to me, the verb "switch" needs two things to switch
> between, i.e. "switch A and B", unless it is "switch to X".
> Either "switch-to-branch" or simply "switch-to", perhaps?
>
> As I already hinted in my response to Stefan (?) about
> checkout-from-tree vs checkout-from-index, a command with multiple
> modes of operation is not confusing to people with the right mental
> model, and I suspect that having two separate commands for "checking
> out a branch" and "checking out paths" that is done by this step
> would help users to form the right mental model.

Since the other one is already "checkout-files", maybe this one could
just be "checkout-branch".

> So I tend to think
> these two are "training wheels", and suspect that once they got it,
> nobody will become "sick of" the single "checkout" command that can
> be used to do either.  It's just the matter of being aware what can
> be done (which requires the right mental model) and how to tell Git
> what the user wants it do (two separate commands, operating mode
> option, or just the implied command line syntax---once the user
> knows what s/he is doing, these do not make that much a difference).

I would hope this becomes better defaults and being used 90% of time.
Even though I know "git checkout" quite well, it still bites me from
time to time. Having the right mental model is one thing. Having to
think a bit every time to write "git checkout" with the right syntax,
and whether you need "--" (that ambiguation problem can still bite you
from time to time), is frankly something I'd rather avoid.
-- 
Duy

Re: [PATCH v2 1/7] parse-options: allow parse_options_concat(NULL, options)

[Duy Nguyen]

On Tue, Nov 27, 2018 at 8:44 PM Stefan Beller  wrote:
>
> On Tue, Nov 27, 2018 at 8:53 AM Nguyễn Thái Ngọc Duy  
> wrote:
> >
> > There is currently no caller that calls this function with "a" being
> > NULL. But it will be introduced shortly. It is used to construct the
> > option array from scratch, e.g.
> >
> >struct parse_options opts = NULL;
> >opts = parse_options_concat(opts, opts_1);
> >opts = parse_options_concat(opts, opts_2);
>
> While this addresses the immediate needs, I'd prefer to think
> about the API exposure of parse_options_concat,
> (related: do we want to have docs in its header file?)
> and I'd recommend to make it symmetrical, i.e.
> allow the second argument to also be NULL?

I'll just drop this patch. There's a better way to do the same without
adding special handling like this.
-- 
Duy

Re: [RFC PATCH] Introduce "precious" file concept

[Duy Nguyen]

On Tue, Nov 27, 2018 at 1:56 PM Jacob Keller  wrote:
>
> On Tue, Nov 27, 2018 at 1:45 AM Per Lundberg  wrote:
> >
> > On 11/26/18 5:55 PM, Duy Nguyen wrote:
> > > On Mon, Nov 26, 2018 at 4:47 PM Ævar Arnfjörð Bjarmason
> > >  wrote:
> > >> Some of the solutions overlap with this thing you want, but I think it's
> > >> worth keeping the distinction between the two in mind.
> > >
> > > On the other hand all use cases should be considered. It's going to be
> > > a mess to have "trashable" attribute that applies to some commands
> > > while "precious" to some others (and even worse when they overlap,
> > > imagine having to define both in .gitattributes)
> >
> > Agree - I think it would be a very bad idea to have a "mix" of both
> > trashable and precious. IMO, we should try to find which one of these
> > concepts suits most general use cases best and causes less churn for
> > existing scripts/users' existing "semantic expectations", and pick that one.
> > --
> > Per Lundberg
>
> Personally, I would rather err on the side which requires the least
> interaction from users to avoid silently clobbering an ignored file.
>
> Either Duy's solution with a sort of "untracked" reflog, or the
> garbage/trashable notion.
>
> I don't like the idea of precious because it means people have to know
> and remember to opt in, and it's quite possible they will not do so
> until after they've lost real data.
>
> I'd only have trashable apply in the case where it was implicit. i.e.
> git clean -fdx would still delete them, as this is an explicit
> operation that (hopefully?) users know will delete data.

Yes I know it will delete ignored files. But I don't want it to delete
some files. There is no way I can tell Git to do that.

It's the same with merge/checkout's overwriting problem. Once the
initial surprise is over, I want control over what files I want Git to
just delete and not annoy me, what Git should not delete.
-- 
Duy

Re: [RFC] Introduce two new commands, switch-branch and restore-paths

[Duy Nguyen]

On Mon, Nov 26, 2018 at 5:01 PM Ævar Arnfjörð Bjarmason
 wrote:
> > So, what do you think?
>
> That "git checkout" does too many things is something that keeps coming
> up in online discussions about Git's UI. Two things:
>
> a) It would really help to have some comparison of cases where these
>split commands are much clearer or less ambiguous than
>git-checkout. I can think of some (e.g. branch with the same name as
>a file) but having some overall picture of what the new UI looks like
>with solved / not solved cases would be nice. Also a comparison with
>other SCMs people find less confusing (svn, hg, bzr, ...)

Less ambiguous is indeed one of the reasons I wanted to do this.

> b) I think we really need to have some end-game where we'd actually
>switch away from "checkout" (which we could still auto-route to new
>commands in perpetuity, but print a warning or error). Otherwise
>we'll just end up with https://xkcd.com/927/ and more UI confusion
>for all.

I'm not going to remove "git checkout". Not until the majority of
users are really in favor of the new ones. So my end-game plan is to
just promote the two new commands in man pages, tutorial, advice...
Perhaps at some point "git checkout" is also removed from "git help".
But that's about it. If you want to stick with "git checkout", it's
there (and not nagging you to move to new ones).
-- 
Duy

Re: [RFC PATCH] Introduce "precious" file concept

[Duy Nguyen]

On Mon, Nov 26, 2018 at 4:47 PM Ævar Arnfjörð Bjarmason
 wrote:
> >> >> How about something like this:
> >> >>
> >> >> 1. Introduce a concept with "garbage" files, which git is "permitted to
> >> >> delete" without prompting.
> >> >>
> >> >> 2. Retain the current default, i.e. "ignored files are garbage" for now,
> >> >> making the new behavior _opt in_ to avoid breaking automated
> >> >> systems/existing scripts for anyone. Put the setting for this behind a
> >> >> new core.* config flag.
> >> >>
> >> >> 3. In the plan for version 3.0 (a new major version where some breakage
> >> >> can be tolerable, according to Semantic Versioning), change the default
> >> >> so that "only explicit garbage is garbage". Include very clear notices
> >> >> of this in the release notes. The config flag is retained, but its
> >> >> default changes from true->false or vice versa. People who dislike the
> >> >> new behavior can easily change back to the 2.x semantics.
> >> >
> >> > How does this garbage thing interact with "git clean -x"? My
> >> > interpretation of this flag/attribute is that at version 3.0 by
> >> > default all ignored files are _not_ garbage, so "git clean -x" should
> >> > not remove any of them. Which is weird because most of ignored files
> >> > are like *.o that should be removed.
> >> >
> >> > I also need to mark "precious" on untracked or even tracked files (*).
> >> > Not sure how this "garbage" attribute interacts with that.
> >> >
> >> > (*) I was hoping I could get the idea [1] implemented in somewhat good
> >> > shape before presenting here. But I'm a bit slow on that front. So
> >> > yeah this "precious" on untracked/tracked thingy may be even
> >> > irrelevant if the patch series will be rejected.
> >>
> >> I think a garbage (or trashable) flag, if implemented, wouldn't need any
> >> special case in git-clean, i.e. -x would remove all untracked files,
> >> whether ignored or garbage/trashable. That's what my patch to implement
> >> it does:
> >> https://public-inbox.org/git/87zhuf3gs0@evledraar.gmail.com/
> >>
> >> I think that makes sense. Users running "git clean" have "--dry-run" and
> >> unlike "checkout a branch" or "merge this commit" where we'll now shred
> >> data implicitly it's obvious that git-clean is going to shred your data.
> >
> > Then that't not what I want. If I'm going to mark to keep "config.mak"
> > around, I'm not going to carefully move it away before doing "git
> > clean -fdx" then move it back. No "git clean --dry-run" telling me to
> > make a backup of config.mak is no good.
>
> Understood. I mean this in the context of solving the problem users have
> with running otherwise non-data-destroying commands like "checkout" and
> "merge" and getting their data destroyed, which is overwhelmingly why
> this topic gets resurrected.
>
> Some of the solutions overlap with this thing you want, but I think it's
> worth keeping the distinction between the two in mind.

On the other hand all use cases should be considered. It's going to be
a mess to have "trashable" attribute that applies to some commands
while "precious" to some others (and even worse when they overlap,
imagine having to define both in .gitattributes)

> I.e. I can
> imagine us finding some acceptable solution to the data shredding
> problem that doesn't implement this mode for "git-clean", or the other
> way around.
-- 
Duy

Re: [RFC PATCH] Introduce "precious" file concept

[Duy Nguyen]

On Mon, Nov 26, 2018 at 4:34 PM Ævar Arnfjörð Bjarmason
 wrote:
>
>
> On Mon, Nov 26 2018, Duy Nguyen wrote:
>
> > On Mon, Nov 26, 2018 at 10:30 AM Per Lundberg  wrote:
> >>
> >> On 11/13/18 1:22 AM, brian m. carlson wrote:
> >> > This is going to totally hose automation.  My last job had files which
> >> > might move from tracked to untracked (a file that had become generated),
> >> > and long-running CI and build systems would need to be able to check out
> >> > one status and switch to the other.  Your proposed change will prevent
> >> > those systems from working, whereas they previously did.
> >> >
> >> > I agree that your proposal would have been a better design originally,
> >> > but breaking the way automated systems currently work is probably going
> >> > to be a dealbreaker.
> >>
> >> How about something like this:
> >>
> >> 1. Introduce a concept with "garbage" files, which git is "permitted to
> >> delete" without prompting.
> >>
> >> 2. Retain the current default, i.e. "ignored files are garbage" for now,
> >> making the new behavior _opt in_ to avoid breaking automated
> >> systems/existing scripts for anyone. Put the setting for this behind a
> >> new core.* config flag.
> >>
> >> 3. In the plan for version 3.0 (a new major version where some breakage
> >> can be tolerable, according to Semantic Versioning), change the default
> >> so that "only explicit garbage is garbage". Include very clear notices
> >> of this in the release notes. The config flag is retained, but its
> >> default changes from true->false or vice versa. People who dislike the
> >> new behavior can easily change back to the 2.x semantics.
> >
> > How does this garbage thing interact with "git clean -x"? My
> > interpretation of this flag/attribute is that at version 3.0 by
> > default all ignored files are _not_ garbage, so "git clean -x" should
> > not remove any of them. Which is weird because most of ignored files
> > are like *.o that should be removed.
> >
> > I also need to mark "precious" on untracked or even tracked files (*).
> > Not sure how this "garbage" attribute interacts with that.
> >
> > (*) I was hoping I could get the idea [1] implemented in somewhat good
> > shape before presenting here. But I'm a bit slow on that front. So
> > yeah this "precious" on untracked/tracked thingy may be even
> > irrelevant if the patch series will be rejected.
>
> I think a garbage (or trashable) flag, if implemented, wouldn't need any
> special case in git-clean, i.e. -x would remove all untracked files,
> whether ignored or garbage/trashable. That's what my patch to implement
> it does:
> https://public-inbox.org/git/87zhuf3gs0@evledraar.gmail.com/
>
> I think that makes sense. Users running "git clean" have "--dry-run" and
> unlike "checkout a branch" or "merge this commit" where we'll now shred
> data implicitly it's obvious that git-clean is going to shred your data.

Then that't not what I want. If I'm going to mark to keep "config.mak"
around, I'm not going to carefully move it away before doing "git
clean -fdx" then move it back. No "git clean --dry-run" telling me to
make a backup of config.mak is no good.
-- 
Duy

Re: [RFC] Introduce two new commands, switch-branch and restore-paths

[Duy Nguyen]

On Mon, Nov 26, 2018 at 4:03 AM Junio C Hamano  wrote:
>
> Thomas Gummerer  writes:
>
> > I like the idea of splitting those commands up, in fact it is
> > something I've been considering working on myself.  I do think we
> > should consider if we want to change the behaviour of those new
> > commands in any way compared to 'git checkout', since we're starting
> > with a clean slate.

Better defaults? Hell yes!

> > One thing in particular that I have in mind is something I'm currently
> > working on, namely adding a --index flag to 'git checkout', which
> > would make 'git checkout' work in non-overlay mode (for more
> > discussion on that see also [*1*].
>
> Ah, thanks for reminding me of that.  That explains why I felt
> uneasy to see "restore" in the proposed command name.

About that name. I didn't want to start the command name with checkout
to avoid completion conflict (the obvious choice was checkout-path,
the function name behind it). And I didn't find any other good name,
so I picked "restore" out of git-checkout.txt's one-line description.
If we end up with a better command name, perhaps reword that line too.
-- 
Duy

Re: [RFC PATCH] Introduce "precious" file concept

[Duy Nguyen]

On Mon, Nov 26, 2018 at 10:30 AM Per Lundberg  wrote:
>
> On 11/13/18 1:22 AM, brian m. carlson wrote:
> > This is going to totally hose automation.  My last job had files which
> > might move from tracked to untracked (a file that had become generated),
> > and long-running CI and build systems would need to be able to check out
> > one status and switch to the other.  Your proposed change will prevent
> > those systems from working, whereas they previously did.
> >
> > I agree that your proposal would have been a better design originally,
> > but breaking the way automated systems currently work is probably going
> > to be a dealbreaker.
>
> How about something like this:
>
> 1. Introduce a concept with "garbage" files, which git is "permitted to
> delete" without prompting.
>
> 2. Retain the current default, i.e. "ignored files are garbage" for now,
> making the new behavior _opt in_ to avoid breaking automated
> systems/existing scripts for anyone. Put the setting for this behind a
> new core.* config flag.
>
> 3. In the plan for version 3.0 (a new major version where some breakage
> can be tolerable, according to Semantic Versioning), change the default
> so that "only explicit garbage is garbage". Include very clear notices
> of this in the release notes. The config flag is retained, but its
> default changes from true->false or vice versa. People who dislike the
> new behavior can easily change back to the 2.x semantics.

How does this garbage thing interact with "git clean -x"? My
interpretation of this flag/attribute is that at version 3.0 by
default all ignored files are _not_ garbage, so "git clean -x" should
not remove any of them. Which is weird because most of ignored files
are like *.o that should be removed.

I also need to mark "precious" on untracked or even tracked files (*).
Not sure how this "garbage" attribute interacts with that.

(*) I was hoping I could get the idea [1] implemented in somewhat good
shape before presenting here. But I'm a bit slow on that front. So
yeah this "precious" on untracked/tracked thingy may be even
irrelevant if the patch series will be rejected.

[1] 
https://public-inbox.org/git/cacsjy8c3rofv0kqejrwufqqzbnfu4msxjtpheybgmmrroff...@mail.gmail.com/

> Would this be a reasonable compromise for everybody?
-- 
Duy

Re: [PATCH] files-backend.c: fix build error on Solaris

[Duy Nguyen]

On Sun, Nov 25, 2018 at 11:19 AM Carlo Arenas  wrote:
>
> Signed-off-by: Carlo Marcelo Arenas Belón 
>
> clang with -Wpedantic also catch this (at least with Apple LLVM
> version 10.0.0); recent versions of gcc also include that flag and at
> least 8.2.0 shows a warning for it, so it might be worth adding it to
> developer mode (maybe under the pedantic DEVOPTS), would this be
> something I should be adding?, what are the expectations around
> warnings and compilers?

make DEVELOPER=1 DEVOPTS=pedantic (with either gcc 7.3.0 or 8.2.0) can
already catch this. I have no comment if this pedantic should be part
of default DEVELOPER=1. But at least in controlled environments like
Travis, we could turn it on to catch more.
-- 
Duy

Re: [PATCH] clone: fix colliding file detection on APFS

[Duy Nguyen]

On Tue, Nov 20, 2018 at 8:35 PM Carlo Arenas  wrote:
> IMHO it would be ideal if test would be enabled/validated for windows
> (native, not only cygwin) as it might even work without the override
> and if we are to see conflicts, that is probably where most users with
> file insensitive filesystems might be found

Yes but I can't test on Windows so I will not enable the test until I
got a report that it's working there.
-- 
Duy

[RFC] Introduce two new commands, switch-branch and restore-paths

[Duy Nguyen]

On Mon, Nov 19, 2018 at 04:19:53PM +0100, Duy Nguyen wrote:
> I promise to come back with something better (at least it still
> sounds better in my mind). If that idea does not work out, we can
> come back and see if we can improve this.

So this is it. The patch isn't pretty, mostly as a proof of
concept. Just look at the three functions at the bottom of checkout.c,
which is the main thing.

This patch tries to split "git checkout" command in two new ones:

- git switch-branch is all about switching branches
- git restore-paths (maybe restore-file is better) for checking out
  paths

The main idea is these two commands will co-exist with the good old
'git checkout', which will NOT be deprecated. Old timers will still
use "git checkout". But new people should be introduced to the new two
instead. And the new ones are just as capable as "git checkout".

Since the three commands will co-exist (with duplicate functionality),
maintenance cost must be kept to minimum. The way I did this is simply
split the command line options into three pieces: common,
switch-branch and checkout-paths. "git checkout" has all three, the
other two have common and another piece.

With this, a new option added to git checkout will be automatically
available in either switch-branch or checkout-paths. Bug fixes apply
to all relevant commands.

Later on, we could start to add a bit more stuff in, e.g. some form of
disambiguation is no longer needed when running as switch-branch, or
restore-paths.

So, what do you think?

-- 8< --
diff --git a/builtin.h b/builtin.h
index 6538932e99..6e321ec8a4 100644
--- a/builtin.h
+++ b/builtin.h
@@ -214,6 +214,7 @@ extern int cmd_remote_fd(int argc, const char **argv, const 
char *prefix);
 extern int cmd_repack(int argc, const char **argv, const char *prefix);
 extern int cmd_rerere(int argc, const char **argv, const char *prefix);
 extern int cmd_reset(int argc, const char **argv, const char *prefix);
+extern int cmd_restore_paths(int argc, const char **argv, const char *prefix);
 extern int cmd_rev_list(int argc, const char **argv, const char *prefix);
 extern int cmd_rev_parse(int argc, const char **argv, const char *prefix);
 extern int cmd_revert(int argc, const char **argv, const char *prefix);
@@ -227,6 +228,7 @@ extern int cmd_show_index(int argc, const char **argv, 
const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_submodule__helper(int argc, const char **argv, const char 
*prefix);
+extern int cmd_switch_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
 extern int cmd_tag(int argc, const char **argv, const char *prefix);
 extern int cmd_tar_tree(int argc, const char **argv, const char *prefix);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index acdafc6e4c..868ca3c223 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -33,6 +33,16 @@ static const char * const checkout_usage[] = {
NULL,
 };
 
+static const char * const switch_branch_usage[] = {
+   N_("git switch-branch [] "),
+   NULL,
+};
+
+static const char * const restore_paths_usage[] = {
+   N_("git restore-paths [] [] -- ..."),
+   NULL,
+};
+
 struct checkout_opts {
int patch_mode;
int quiet;
@@ -44,6 +54,7 @@ struct checkout_opts {
int ignore_skipworktree;
int ignore_other_worktrees;
int show_progress;
+   int dwim_new_local_branch;
/*
 * If new checkout options are added, skip_merge_working_tree
 * should be updated accordingly.
@@ -55,6 +66,7 @@ struct checkout_opts {
int new_branch_log;
enum branch_track track;
struct diff_options diff_options;
+   char *conflict_style;
 
int branch_exists;
const char *prefix;
@@ -1223,78 +1235,105 @@ static int checkout_branch(struct checkout_opts *opts,
return switch_branches(opts, new_branch_info);
 }
 
-int cmd_checkout(int argc, const char **argv, const char *prefix)
+static struct option *add_common_options(struct checkout_opts *opts,
+struct option *prevopts)
 {
-   struct checkout_opts opts;
-   struct branch_info new_branch_info;
-   char *conflict_style = NULL;
-   int dwim_new_local_branch = 1;
-   int dwim_remotes_matched = 0;
struct option options[] = {
-   OPT__QUIET(, N_("suppress progress reporting")),
-   OPT_STRING('b', NULL, _branch, N_("branch"),
+   OPT__QUIET(>quiet, N_("suppress progress reporting")),
+   OPT_BOOL(0, "ignore-skip-worktree-bits", 
>ignore_skipworktree,
+N_("do not limit pathspecs to sparse entries only")),
+

Re: [PATCH 1/3] pack-objects: fix tree_depth and layer invariants

[Duy Nguyen]

On Tue, Nov 20, 2018 at 11:04 AM Jeff King  wrote:
>
> Commit 108f530385 (pack-objects: move tree_depth into 'struct
> packing_data', 2018-08-16) dynamically manages a tree_depth array in
> packing_data that maintains one of these invariants:
>
>   1. tree_depth is NULL (i.e., the requested options don't require us to
>  track tree depths)
>
>   2. tree_depth is non-NULL and has as many entries as the "objects"
>  array
>
> We maintain (2) by:
>
>   a. When the objects array grows, grow tree_depth to the same size
>  (unless it's NULL, in which case we can leave it).
>
>   b. When a caller asks to set a depth via oe_set_tree_depth(), if
>  tree_depth is NULL we allocate it.
>
> But in (b), we use the number of stored objects, _not_ the allocated
> size of the objects array. So we can run into a situation like this:
>
>   1. packlist_alloc() needs to store the Nth object, so it grows the
>  objects array to M, where M > N.
>
>   2. oe_set_tree_depth() wants to store a depth, so it allocates an
>  array of length N. Now we've violated our invariant.
>
>   3. packlist_alloc() needs to store the N+1th object. But it _doesn't_
>  grow the objects array, since N <= M still holds. We try to assign
>  to tree_depth[N+1], which is out of bounds.

Do you think if this splitting data to packing_data is too fragile
that we should just scrape the whole thing and move all data back to
object_entry[]? We would use more memory of course but higher memory
usage is still better than more bugs (if these are likely to show up
again).
-- 
Duy

Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems

[Duy Nguyen]

On Mon, Nov 19, 2018 at 10:03 PM Duy Nguyen  wrote:
> Thanks Carlo for the file and "stat" output. The problem is APFS has
> 64-bit inode (according to the Internet) while we store inodes as
> 32-bit, so it's truncated.
> ...
> We will have to deal with the same
> truncated inode elsewhere to make sure we index refresh performance
> does not degrade on APFS.

... and we don't have a problem there. Either Linus predicted dealing
with 64-bit inodes, or he had a habit of casting st_ino to unsigned
int, I cannot tell. This code

ce->st_ino != (unsigned int)st->st_ino

is from e83c516331 (Initial revision of "git", the information manager
from hell - 2005-04-07) and it's still used today for comparing sd_ino
with st->st_ino in read-cache.c. I guess I should have copied and
pasted more often.
-- 
Duy

Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems

[Duy Nguyen]

... and I "dear Ramsay" without CCing him.. sigh.. sorry for the noise.

On Mon, Nov 19, 2018 at 10:03 PM Duy Nguyen  wrote:
>
> First of all, Ramsay, it would be great if you could test the below
> patch and see if it works on Cygwin. I assume since Cygwin shares the
> underlying filesystem, it will share the same "no trusting inode"
> issue with native builds (or it calculates inodes anyway using some
> other source?).
>
> Back to the APFS problem...
>
> On Mon, Nov 19, 2018 at 07:24:26PM +0100, Duy Nguyen wrote:
> > Could you send me the "index" file in  t/trash\
> > directory.t5601-clone/icasefs/bogus/.git/index ? Also the output of
> > "stat /path/to/icase/bogus/x"
> >
> > My only explanation is somehow the inode value we save is not the same
> > one on disk, which is weird and could even cause other problems. I'd
> > like to know why this happens before trying to fix anything.
>
> Thanks Carlo for the file and "stat" output. The problem is APFS has
> 64-bit inode (according to the Internet) while we store inodes as
> 32-bit, so it's truncated. Which means this comparison
>
> sd_ino == st_ino
>
> is never true because sd_ino is truncated (0x2121063) while st_ino is
> not (0x202121063).
>
> Carlo, it would be great if you could test this patch also with
> APFS. It should fix problem. We will have to deal with the same
> truncated inode elsewhere to make sure we index refresh performance
> does not degrade on APFS. But that's a separate problem. Thank you for
> bringing this up.
>
> -- 8< --
> diff --git a/entry.c b/entry.c
> index 5d136c5d55..809d3e2ba7 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -404,13 +404,13 @@ static void mark_colliding_entries(const struct 
> checkout *state,
>  {
> int i, trust_ino = check_stat;
>
> -#if defined(GIT_WINDOWS_NATIVE)
> +#if defined(GIT_WINDOWS_NATIVE) || defined(__CYGWIN__)
> trust_ino = 0;
>  #endif
>
> ce->ce_flags |= CE_MATCHED;
>
> -   for (i = 0; i < state->istate->cache_nr; i++) {
> +   for (i = 0; i < trust_ino && state->istate->cache_nr; i++) {
> struct cache_entry *dup = state->istate->cache[i];
>
> if (dup == ce)
> @@ -419,10 +419,24 @@ static void mark_colliding_entries(const struct 
> checkout *state,
> if (dup->ce_flags & (CE_MATCHED | CE_VALID | 
> CE_SKIP_WORKTREE))
> continue;
>
> -   if ((trust_ino && dup->ce_stat_data.sd_ino == st->st_ino) ||
> -   (!trust_ino && !fspathcmp(ce->name, dup->name))) {
> +   if (dup->ce_stat_data.sd_ino == (unsigned int)st->st_ino) {
> dup->ce_flags |= CE_MATCHED;
> +   return;
> +   }
> +   }
> +
> +   for (i = 0; i < state->istate->cache_nr; i++) {
> +   struct cache_entry *dup = state->istate->cache[i];
> +
> +   if (dup == ce)
> break;
> +
> +   if (dup->ce_flags & (CE_MATCHED | CE_VALID | 
> CE_SKIP_WORKTREE))
> +   continue;
> +
> +   if (!fspathcmp(ce->name, dup->name)) {
> +   dup->ce_flags |= CE_MATCHED;
> +   return;
> }
> }
>  }
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index f1a49e94f5..c28d51bd59 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -628,7 +628,7 @@ test_expect_success 'clone on case-insensitive fs' '
> )
>  '
>
> -test_expect_success !MINGW,!CYGWIN,CASE_INSENSITIVE_FS 'colliding file 
> detection' '
> +test_expect_success !MINGW,CASE_INSENSITIVE_FS 'colliding file detection' '
> grep X icasefs/warning &&
> grep x icasefs/warning &&
> test_i18ngrep "the following paths have collided" icasefs/warning
> -- 8< --



-- 
Duy

Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems

[Duy Nguyen]

First of all, Ramsay, it would be great if you could test the below
patch and see if it works on Cygwin. I assume since Cygwin shares the
underlying filesystem, it will share the same "no trusting inode"
issue with native builds (or it calculates inodes anyway using some
other source?).

Back to the APFS problem...

On Mon, Nov 19, 2018 at 07:24:26PM +0100, Duy Nguyen wrote:
> Could you send me the "index" file in  t/trash\
> directory.t5601-clone/icasefs/bogus/.git/index ? Also the output of
> "stat /path/to/icase/bogus/x"
> 
> My only explanation is somehow the inode value we save is not the same
> one on disk, which is weird and could even cause other problems. I'd
> like to know why this happens before trying to fix anything.

Thanks Carlo for the file and "stat" output. The problem is APFS has
64-bit inode (according to the Internet) while we store inodes as
32-bit, so it's truncated. Which means this comparison

sd_ino == st_ino

is never true because sd_ino is truncated (0x2121063) while st_ino is
not (0x202121063).

Carlo, it would be great if you could test this patch also with
APFS. It should fix problem. We will have to deal with the same
truncated inode elsewhere to make sure we index refresh performance
does not degrade on APFS. But that's a separate problem. Thank you for
bringing this up.

-- 8< --
diff --git a/entry.c b/entry.c
index 5d136c5d55..809d3e2ba7 100644
--- a/entry.c
+++ b/entry.c
@@ -404,13 +404,13 @@ static void mark_colliding_entries(const struct checkout 
*state,
 {
int i, trust_ino = check_stat;
 
-#if defined(GIT_WINDOWS_NATIVE)
+#if defined(GIT_WINDOWS_NATIVE) || defined(__CYGWIN__)
trust_ino = 0;
 #endif
 
ce->ce_flags |= CE_MATCHED;
 
-   for (i = 0; i < state->istate->cache_nr; i++) {
+   for (i = 0; i < trust_ino && state->istate->cache_nr; i++) {
struct cache_entry *dup = state->istate->cache[i];
 
if (dup == ce)
@@ -419,10 +419,24 @@ static void mark_colliding_entries(const struct checkout 
*state,
if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))
continue;
 
-   if ((trust_ino && dup->ce_stat_data.sd_ino == st->st_ino) ||
-   (!trust_ino && !fspathcmp(ce->name, dup->name))) {
+   if (dup->ce_stat_data.sd_ino == (unsigned int)st->st_ino) {
dup->ce_flags |= CE_MATCHED;
+   return;
+   }
+   }
+
+   for (i = 0; i < state->istate->cache_nr; i++) {
+   struct cache_entry *dup = state->istate->cache[i];
+
+   if (dup == ce)
break;
+
+   if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))
+   continue;
+
+   if (!fspathcmp(ce->name, dup->name)) {
+   dup->ce_flags |= CE_MATCHED;
+   return;
}
}
 }
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index f1a49e94f5..c28d51bd59 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -628,7 +628,7 @@ test_expect_success 'clone on case-insensitive fs' '
)
 '
 
-test_expect_success !MINGW,!CYGWIN,CASE_INSENSITIVE_FS 'colliding file 
detection' '
+test_expect_success !MINGW,CASE_INSENSITIVE_FS 'colliding file detection' '
grep X icasefs/warning &&
grep x icasefs/warning &&
test_i18ngrep "the following paths have collided" icasefs/warning
-- 8< --

Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems

[Duy Nguyen]

On Mon, Nov 19, 2018 at 6:14 PM Carlo Arenas  wrote:
>
> On Mon, Nov 19, 2018 at 4:28 AM Torsten Bögershausen  wrote:
> >
> > Did you test it on Mac ?
>
> macOS 10.14.1 but only using APFS, did you test my patch with HFS+?
>
> > So what exactly are you trying to fix ?
>
> I get
>
> not ok 99 - colliding file detection
> #
> # grep X icasefs/warning &&
> # grep x icasefs/warning &&
> # test_i18ngrep "the following paths have collided" icasefs/warning
> #
>
> and the output of "warning" only shows one of the conflicting files,
> instead of both:
>
> Cloning into 'bogus'...
> done.
> warning: the following paths have collided (e.g. case-sensitive paths
> on a case-insensitive filesystem) and only one from the same
> colliding group is in the working tree:
>
>   'x'
>
> Carlo

Could you send me the "index" file in  t/trash\
directory.t5601-clone/icasefs/bogus/.git/index ? Also the output of
"stat /path/to/icase/bogus/x"

My only explanation is somehow the inode value we save is not the same
one on disk, which is weird and could even cause other problems. I'd
like to know why this happens before trying to fix anything.
-- 
Duy

Re: [PATCH 5/5] tree-walk: support :(attr) matching

[Duy Nguyen]

On Sun, Nov 18, 2018 at 8:58 PM Ævar Arnfjörð Bjarmason
 wrote:
>
>
> On Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote:
>
> As noted in
> https://public-inbox.org/git/87d0r217vr@evledraar.gmail.com/ I'm
> happy to see this implemented. I have not read this patch in much
> detail...
>
> > [...]
> >  Documentation/glossary-content.txt |  2 +
> > [...]
> > diff --git a/Documentation/glossary-content.txt 
> > b/Documentation/glossary-content.txt
> > index 0d2aa48c63..023ca95e7c 100644
> > --- a/Documentation/glossary-content.txt
> > +++ b/Documentation/glossary-content.txt
> > @@ -404,6 +404,8 @@ these forms:
> >  - "`!ATTR`" requires that the attribute `ATTR` be
> >unspecified.
> >  +
> > +Note that when matching against a tree object, attributes are still
> > +obtained from working tree, not from the given tree object.
> >
> >  exclude;;
> >   After a path matches any non-exclude pathspec, it will be run
>
> Just a poke again about what I brought up in the thread you replied to
> in
> https://public-inbox.org/git/cacsjy8clhq0mkhkxvtdaqy9tlwefbsvheu5ubpxhx4is2mk...@mail.gmail.com/
>
> I.e. the documentation of these various wildmatch() / attributes
> patterns we support is all over the place. Some in gitignore(5), some
> not documented at all, and some in gitglossary(7) (which really should
> not be serving as primary documentation for anything).

Poking me is not going help, I'm afraid. Except bugs, which have
highest priority to me, I do other stuff first either because I need
it or it's fun to do, and this wildmatch documentation is neither (and
I have a long backlog).
-- 
Duy

Re: [PATCH 0/5] Make :(attr) pathspec work with "git log"

[Duy Nguyen]

On Mon, Nov 19, 2018 at 12:42 PM Ævar Arnfjörð Bjarmason
 wrote:
>
>
> On Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote:
>
> > When :(attr) was added, it supported one of the two main pathspec
> > matching functions, the one that works on a list of paths. The other
> > one works on a tree, tree_entry_interesting(), which gets :(attr)
> > support in this series.
> >
> > With this, "git grep   -- :(attr)" or "git log :(attr)"
> > will not abort with BUG() anymore.
> >
> > But this also reveals an interesting thing: even though we walk on a
> > tree, we check attributes from _worktree_ (and optionally fall back to
> > the index). This is how attributes are implemented since forever. I
> > think this is not a big deal if we communicate clearly with the user.
> > But otherwise, this series can be scraped, as reading attributes from
> > a specific tree could be a lot of work.
> >
> > The main patch is the last one. The others are just to open a path to
> > pass "struct index_state *" down to tree_entry_interesting(). This may
> > become standard procedure because we don't want to stick the_index (or
> > the_repository) here and there.
>
> Another side-note (this thread is turning into my personal blog at this
> point...) I found an old related thread:
> https://public-inbox.org/git/20170509225219.gb106...@google.com/
>
> So this series fixes 1/2 of the issues noted there, but git-ls-tree will
> still die with the same error.

If you mean BUG(), not it does not. There are just a couple more
places besides tree_entry_interesting() and match_pathspec() that need
to be aware of all the magic things. ls-tree is one of those but it's
well guarded and will display this instead

$ git ls-tree HEAD ':(attr:abc=def)'
fatal: :(attr:abc=def): pathspec magic not supported by this command: 'attr'

If you mean making ls-tree support :(attr) and other stuff, it's not
going to happen in near future. It may be nice to switch to
tree_entry_interesting() in this command, but if it was easy to do I
would have done it years ago :P
-- 
Duy

Re: [PATCH 0/5] Make :(attr) pathspec work with "git log"

[Duy Nguyen]

On Mon, Nov 19, 2018 at 12:16 PM Ævar Arnfjörð Bjarmason
 wrote:
> As an aside, how do you do the inverse of matching for an attribute by
> value? I.e.:
>
> $ git ls-files | wc -l; git ls-files ':(attr:diff=perl)' | wc -l
> 3522
> 65
>
> I'd like something gives me all files that don't match diff=perl,
> i.e. 3522-65 = 3457 files, or what I'd get if I constructed such a match
> manually with excludes:
>
> $ git ls-files $(grep diff=perl .gitattributes | cut -d ' ' -f1 | sed 
> 's!^!:(exclude)!') | wc -l
> 3457
>
> From my reading of parse_pathspec_attr_match() and match_attrs() this
> isn't possible and I'd need to support ':(attr:diff!=perl)' via a new
> MATCH_NOT_VALUE mode. But I wanted to make sure I wasn't missing some
> subtlety, i.e. that this was implemented already via some other feature.
>
> I thought I could do:
>
> git ls-files ':(exclude):(attr:diff=perl)'
>
> But we don't support chaining like that, and this would only exclude a
> file that's actually called ":(attr:diff=perl)". I.e. created via
> something like "touch ':(attr:diff=perl)'".

I think we allow :(exclude,attr:diff=perl) which should "exclude all
paths that have diff=perl attribute". It's actually tested in t6135
for ls-files (but I didn't add the same test for 'git grep' because I
was so confident it would work; I'll work on that).
-- 
Duy

Re: [PATCH/RFC] checkout: print something when checking out paths

[Duy Nguyen]

On Mon, Nov 19, 2018 at 2:08 PM Ævar Arnfjörð Bjarmason
 wrote:
>
>
> On Mon, Nov 12 2018, Duy Nguyen wrote:
>
> > On Mon, Nov 12, 2018 at 7:21 AM Junio C Hamano  wrote:
> >>
> >> Nguyễn Thái Ngọc Duy   writes:
> >>
> >> > Since the purpose of printing this is to help disambiguate. Only do it
> >> > when "--" is missing (the actual reason though is many tests check
> >> > empty stderr to see that no error is raised and I'm too lazy to fix
> >> > all the test cases).
> >>
> >> Heh, honesty is good but in this particular case the official reason
> >> alone would make perfect sense, too ;-)
> >>
> >> As with progress output, shouldn't this automatically be turned off
> >> when the standard error stream goes to non tty, as the real purpose
> >> of printing is to help the user sitting in front of the terminal and
> >> interacting with the command?
> >
> > I see this at the same level as "Switched to branch 'foo'" which is
> > also protected by opts->quiet. If we start hiding messages based on
> > tty it should be done for other messages in update_refs_for_switch()
> > too, I guess?
>
> I have no real opinion either way, but whatever we can do about
> "checkout" being so confusing since it does so many things is most
> welcome.
>
> Just an alternative: Maybe we can start this out as advice() output
> that's either opt-in via config (not on by default) to start with, or
> have some advice_tty() that only emits it in the same circumstances we
> emit the progress output?

No let's drop this for now. I promise to come back with something
better (at least it still sounds better in my mind). If that idea does
not work out, we can come back and see if we can improve this.
-- 
Duy

Re: [PATCH] technical doc: add a design doc for the evolve command

[Duy Nguyen]

On Thu, Nov 15, 2018 at 2:00 AM  wrote:
> +Goals
> +-
> +Legend: Goals marked with P0 are required. Goals marked with Pn should be
> +attempted unless they interfere with goals marked with Pn-1.
> +
> +P0. All commands that modify commits (such as the normal commit --amend or
> +rebase command) should mark the old commit as being obsolete and 
> replaced by
> +the new one. No additional commands should be required to keep the
> +obsolescence graph up-to-date.

I sometimes "modify" a commit by "git reset @^", pick up the changes
then "git commit -c @{1}". I don't think this counts as a typical
modification and is probably hard to detect automatically. But I hope
there's some way for me to tell git "yes this is a modified commit of
that one, record that!".

> +Example usage
> +-
> +# First create three dependent changes
> +$ echo foo>bar.txt && git add .
> +$ git commit -m "This is a test"
> +created change metas/this_is_a_test

I guess as an example, how the name metas/this_is_a_test is
constructed does not matter much. But it's probably better to stick
with some sort of id because subject line will change over time and
the original one may become irrelevant. Perhaps we could use the
original commit id as name.

> +$ echo foo2>bar2.txt && git add .
> +$ git commit -m "This is also a test"
> +created change metas/this_is_also_a_test
> +$ echo foo3>bar3.txt && git add .
> +$ git commit -m "More testing"
> +created change metas/more_testing
> +
> +# List all our changes in progress
> +$ git change -l
> +metas/this_is_a_test
> +metas/this_is_also_a_test
> +* metas/more_testing
> +metas/some_change_already_merged_upstream
> +
> +# Now modify the earliest change, using its stable name
> +$ git reset --hard metas/this_is_a_test
> +$ echo morefoo>>bar.txt && git add . && git commit --amend --no-edit
> +
> +# Use git-evolve to fix up any dependent changes
> +$ git evolve
> +rebasing metas/this_is_also_a_test onto metas/this_is_a_test
> +rebasing metas/more_testing onto metas/this_is_also_a_test
> +Done
> +
> +# Use git-obslog to view the history of the this_is_a_test change
> +$ git obslog
> +93f110 metas/this_is_a_test@{0} commit (amend): This is a test
> +930219 metas/this_is_a_test@{1} commit: This is a test
> +
> +# Now create an unrelated change
> +$ git reset --hard origin/master
> +$ echo newchange>unrelated.txt && git add .
> +$ git commit -m "Unrelated change"
> +created change metas/unrelated_change
> +
> +# Fetch the latest code from origin/master and use git-evolve
> +# to rebase all dependent changes.
> +$ git fetch origin master
> +$ git evolve origin/master
> +deleting metas/some_change_already_merged_upstream
> +rebasing metas/this_is_a_test onto origin/master
> +rebasing metas/this_is_also_a_test onto metas/this_is_a_test
> +rebasing metas/more_testing onto metas/this_is_also_a_test
> +rebasing metas/unrelated_change onto origin/master
> +Conflict detected! Resolve it and then use git evolve --continue to resume.
> +
> +# Sort out the conflict
> +$ git mergetool
> +$ git evolve --continue
> +Done
> +
> +# Share the full history of edits for the this_is_a_test change
> +# with a review server
> +$ git push origin metas/this_is_a_test:refs/for/master
> +# Share the lastest commit for “Unrelated change”, without history
> +$ git push origin HEAD:refs/for/master

How do we group changes of a topic together? I think branch-diff could
take advantage of that.

> +Detailed design
> +===
> +Obsolescence information is stored as a graph of meta-commits. A meta-commit 
> is
> +a specially-formatted merge commit that describes how one commit was created
> +from others.
> +
> +Meta-commits look like this:
> +
> +$ git cat-file -p 
> +tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904
> +parent aa7ce55545bf2c14bef48db91af1a74e2347539a
> +parent d64309ee51d0af12723b6cb027fc9f195b15a5e9
> +parent 7e1bbcd3a0fa854a7a9eac9bf1eea6465de98136
> +author Stefan Xenos  1540841596 -0700
> +committer Stefan Xenos  1540841596 -0700
> +parent-type content
> +parent-type obsolete
> +parent-type origin
> +
> +This says “commit aa7ce555 makes commit d64309ee obsolete. It was created by
> +cherry-picking commit 7e1bbcd3”.

This feels a bit forced. Could we just organize it like a normal
history? Something like

*
|\
| * last version of the commit
*
|\
| * second last version of the commit
*
|\

Basically all commits will be linked in a new merge history. Real
commits are on the second parent, first parent is to link changes
together. This makes it possible to just use "git log --first-parent
--patch" (or "git log --oneline --graph") to examine the change. More
details (e.g. parent-type) could be stored as normal trailers in the
commit message of these merges.
-- 
Duy

Re: [RFC/PATCH] read-cache: write all indexes with the same permissions

[Duy Nguyen]

On Fri, Nov 16, 2018 at 8:07 PM SZEDER Gábor  wrote:
>
> On Fri, Nov 16, 2018 at 06:41:43PM +0100, Christian Couder wrote:
> > On Tue, Nov 13, 2018 at 6:34 PM Ævar Arnfjörð Bjarmason
> >  wrote:
>
> > > I'm asking whether the bug in this patch isn't revealing an existing
> > > issue with us not having any tests for N number of sharedindex.*
> > > files. I.e. we have >1 of them, merge them and prune them, don't we?
>
> We don't merge shared index files, but write a new one.

True. They are immutable like git objects.

> With the default 20% threshold a new shared index is written rather
> frequently with our usual small test-repos:

Side note. Split index is definitely not meant for small repos. But
maybe we should have a lower limit (in terms of absolute number of
entries) that prevent splitting. This splitting seems excessive.

>   $ git init
>   $ git update-index --split-index
>   $ ls -1 .git/*index*
>   .git/index
>   .git/sharedindex.4370042739b31cd17a5c5cd6043a77c9a00df113
>   $ echo 1 >file
>   $ git add file
>   $ git commit -q -m 1
>   $ echo 2 >file
>   $ git commit -q -m 2 file
>   $ echo 3 >file
>   $ git commit -q -m 3 file
>   $ ls -1 .git/*index*
>   .git/index
>   .git/sharedindex.4370042739b31cd17a5c5cd6043a77c9a00df113
>   .git/sharedindex.6aedbf71b1a6bdc0018078ec7571e1b21ba4b954
>   .git/sharedindex.b9106e9b82a818a0e4e9148224fc44ea98f488a1
>   .git/sharedindex.bad0b75d270a431b9e961cfc15df6ec935a67be8
-- 
Duy

Re: [PATCH v2] read-cache: write all indexes with the same permissions

[Duy Nguyen]

On Fri, Nov 16, 2018 at 8:10 PM Christian Couder
 wrote:
>
> On Fri, Nov 16, 2018 at 7:03 PM Duy Nguyen  wrote:
> >
> > On Fri, Nov 16, 2018 at 6:31 PM Christian Couder
> >  wrote:
> > > diff --git a/read-cache.c b/read-cache.c
> > > index 8c924506dd..ea80600bff 100644
> > > --- a/read-cache.c
> > > +++ b/read-cache.c
> > > @@ -3165,7 +3165,8 @@ int write_locked_index(struct index_state *istate, 
> > > struct lock_file *lock,
> > > struct tempfile *temp;
> > > int saved_errno;
> > >
> > > -   temp = mks_tempfile(git_path("sharedindex_XX"));
> > > +   /* Same permissions as the main .git/index file */
> >
> > If the permission is already correct from the beginning (of this temp
> > file), should df801f3f9f be reverted since we don't need to adjust
> > permission anymore?
>
> df801f3f9f (read-cache: use shared perms when writing shared index,
> 2017-06-25) was fixing the bug that permissions of the shared index
> file did not take into account the shared permissions (which are about
> core.sharedRepository; "shared" has a different meaning in "shared
> index file" and in "shared permissions").
>
> This fix only changes permissions before the shared permissions are
> taken into account (so before adjust_shared_perm() is called).
>
> > Or does $GIT_DIR/index go through the same adjust_shared_perm() anyway
> > in the end, which means df801f3f9f must stay?
>
> Yeah, $GIT_DIR/index goes through adjust_shared_perm() too because
> create_tempfile() calls adjust_shared_perm(). So indeed df801f3f9f
> must stay.

Ah thanks. By the time I got to this part

> Let's instead make the two consistent by using mks_tempfile_sm() and
> passing 0666 in its `mode` argument.

went look at that function and back, I forgot about the paragraph above it.
-- 
Duy

Re: [PATCH v2] read-cache: write all indexes with the same permissions

[Duy Nguyen]

On Fri, Nov 16, 2018 at 6:31 PM Christian Couder
 wrote:
> diff --git a/read-cache.c b/read-cache.c
> index 8c924506dd..ea80600bff 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -3165,7 +3165,8 @@ int write_locked_index(struct index_state *istate, 
> struct lock_file *lock,
> struct tempfile *temp;
> int saved_errno;
>
> -   temp = mks_tempfile(git_path("sharedindex_XX"));
> +   /* Same permissions as the main .git/index file */

If the permission is already correct from the beginning (of this temp
file), should df801f3f9f be reverted since we don't need to adjust
permission anymore?

Or does $GIT_DIR/index go through the same adjust_shared_perm() anyway
in the end, which means df801f3f9f must stay? If so, perhaps clarify
that in the commit message.

> +   temp = mks_tempfile_sm(git_path("sharedindex_XX"), 0, 
> 0666);
> if (!temp) {
> oidclr(>base_oid);
> ret = do_write_locked_index(istate, lock, flags);
-- 
Duy

Re: [PATCH v2 1/1] bundle: cleanup lock files on error

[Duy Nguyen]

On Thu, Nov 15, 2018 at 1:59 PM Johannes Schindelin
 wrote:
> So yes, we are trying to unlink the `.lock` file, and as far as I can tell 
> that
> `unlink()` call comes from the tempfile cleanup asked for by Martin. However, 
> as
> we still have a handle open to that file, that call fails.
>
> I do not think that there is any better way to fix this than to close the file
> explicitly.

I may be talking nonsense here but I remember someone mentioned
something about deleting a file while it's still open on Windows and
after a quick internet search, it may be FILE_SHARE_DELETE. Since _we_
open these files, can we just open them with this to be able to delete
them?
-- 
Duy

Re: [PATCH v2] checkout: print something when checking out paths

[Duy Nguyen]

On Wed, Nov 14, 2018 at 11:12 AM Junio C Hamano  wrote:
>
> Nguyễn Thái Ngọc Duy   writes:
>
> > One of the problems with "git checkout" is that it does so many
> > different things and could confuse people specially when we fail to
> > handle ambiguation correctly.
>
> You would have realized that this is way too noisy if you ran "make
> test", which may have spewed something like this on the screen.

Oh I realize it because it's part of my git build and I often use "git
co ". I'm just telling (or kidding?) myself that I'm just so
used to the old behavior and may need some time to feel comfortable
with the new one.

> [19:09:19] t4120-apply-popt.sh  ok 1624 
> ms ( 0.26 usr  0.21 sys +  5.31 cusr  3.51 csys =  9.29 CPU)
> [19:09:20] t9164-git-svn-dcommit-concurrent.sh  skipped: Perl 
> SVN libraries not found or unusable
> [19:09:20] t1310-config-default.sh  ok  177 
> ms ( 0.07 usr  0.01 sys +  0.89 cusr  0.66 csys =  1.63 CPU)
> ===(   20175;154  1297/?  155/?  6/?  3/3  2/?  4/?  4/?  3/?  5... 
> )===Checked out 1 path out of the index
> Checked out 1 path out of the index
> Checked out 1 path out of the index
> Checked out 1 path out of the index
> Checked out 1 path out of the index
> [19:09:20] t1408-packed-refs.sh ... ok  310 
> ms ( 0.06 usr  0.00 sys +  0.69 cusr  0.52 csys =  1.27 CPU)
> [19:09:20] t0025-crlf-renormalize.sh .. ok  246 
> ms ( 0.03 usr  0.01 sys +  0.34 cusr  0.22 csys =  0.60 CPU)
>
> I am very tempted to suggest to treat this as a training wheel and
> enable only when checkout.showpathcount is set to true, or something
> like that.

Maybe we just drop it then. I'm not adding a training wheel. I'm
trying to make this complex command safer somewhat. But maybe this is
a wrong direction. I'll give the idea "switch-branch / restore-path
alternative commands" a go some time. Then the new generation can just
stick to those and old timers stay with "git checkout".
-- 
Duy

Re: [PATCH v5 02/12] sha1-file: provide functions to look up hash algorithms

[Duy Nguyen]

On Tue, Nov 13, 2018 at 7:42 PM Derrick Stolee  wrote:
>
> On 11/4/2018 6:44 PM, brian m. carlson wrote:
> > +int hash_algo_by_name(const char *name)
> > +{
> > + int i;
> > + if (!name)
> > + return GIT_HASH_UNKNOWN;
> > + for (i = 1; i < GIT_HASH_NALGOS; i++)
> > + if (!strcmp(name, hash_algos[i].name))
> > + return i;
> > + return GIT_HASH_UNKNOWN;
> > +}
> > +
>
> Today's test coverage report [1] shows this method is not covered in the
> test suite. Looking at 'pu', it doesn't have any callers.
>
> Do you have a work in progress series that will use this? Could we add a
> test-tool to exercise this somehow?

Going by the function name, it should be used when hash-object or
other commands learn about --object-format=.
-- 
Duy

Re: [RFC/PATCH] read-cache: write all indexes with the same permissions

[Duy Nguyen]

On Tue, Nov 13, 2018 at 4:32 PM Ævar Arnfjörð Bjarmason
 wrote:
> I won't have time to finish this today, as noted in
> https://public-inbox.org/git/874lcl2e9t@evledraar.gmail.com/
> there's a pretty major bug here in that we're now writing out literal
> sharedindex_XX files.

It's not the end of the world because create_tempfile opens with
O_EXCL so if two processes try to create it at the same time, one will
fail, but no corruption or such.

> Obviously that needs to be fixed, and the fix is trivial, I can use
> another one of the mks_*() functions with the same mode we use to
> create the index.
>
> But we really ought to have tests for the bug this patch introduces,
> and as noted in the E-Mail linked above we don't.
>
> So hopefully Duy or someone with more knowledge of the split index
> will chime in to say what's missing there...

I don't have any bright idea how to catch the literal _X file.
It's a temporary file and will not last long enough for us to verify
unless we intercept open() calls with LD_PRELOAD.
-- 
Duy

Re: [PATCH 2/3] ieot: default to not writing IEOT section

[Duy Nguyen]

On Tue, Nov 13, 2018 at 2:12 AM Jonathan Nieder  wrote:
>
> Junio C Hamano wrote:
>
> > How about
> >
> >   hint: ignoring an optional IEOT extension
> >
> > to make it clear that it is totally harmless?
> >
> > With that, we can add advise.unknownIndexExtension=false to turn all
> > of them off with a single switch.
>
> I like it.  Expect a patch soon (tonight or tomorrow) that does that.
>
> We'll have to find some appropriate place in the documentation to
> explain what the message is about, still.

Also from the last discussion, if I remember correctly, this
"ignoring" is considered harmless and could be suppressed most of the
time. But commands like 'fsck' should always report it.
-- 
Duy

Re: [PATCH] checkout: disambiguate dwim tracking branches and local files

[Duy Nguyen]

On Mon, Nov 12, 2018 at 7:44 AM Junio C Hamano  wrote:
>
> Nguyễn Thái Ngọc Duy   writes:
>
> > @@ -1079,9 +1079,15 @@ static int parse_branchname_arg(int argc, const char 
> > **argv,
> >*/
> >   int recover_with_dwim = dwim_new_local_branch_ok;
> >
> > - if (!has_dash_dash &&
> > - (check_filename(opts->prefix, arg) || !no_wildcard(arg)))
> > - recover_with_dwim = 0;
> > + /*
> > +  * If both refs/remotes/origin/master and the file
> > +  * 'master'. Don't blindly go for 'master' file
> > +  * because it's ambiguous. Leave it for the user to
> > +  * decide.
> > +  */
> > + int disambi_local_file = !has_dash_dash &&
> > + (check_filename(opts->prefix, arg) || 
> > !no_wildcard(arg));
>
> What you are computing on the right hand side is if the arg is
> ambiguous.  And the code that looks at this variable does not
> disambiguate, but it just punts and makes it responsibility to the
> user (which is by the way the correct thing to do).
>
> When a file with exact name is in the working tree, we do not
> declare it is a pathspec and not a rev, as we may be allowed to dwim
> and create a rev with that name and at that point we'd be in an
> ambigous situation.  If the arg _has_ wildcard, however, it is a
> strong sign that it *is* a pathspec, isn't it?  It is iffy that a
> single variable that cannot be used to distinguish these two cases
> is introduced---one of these cases will be mishandled.

Is it that different between an exact path name and a pathspec?
Suppose it is a pathspec (with wildcards) that matches some paths, and
we happen to have the remote branch origin/, then it is
still ambiguous whether we should go create branch
"" or go check out the paths matched by the pathspec.

> Also how does the patch ensures that this new logic does not kick in
> for those who refuse to let the command dwim to create a new branch?

I would hope that it would be "--" to settle all disputes. But it
looks like "git checkout foo --" is explicitly a candidate for dwim.
We have a hidden option --no-guess to disable dwim. Maybe it's a good
idea to make that one visible. It's at least clearer than using "--"
even if "--" worked on this case.

>
> >   /*
> >* Accept "git checkout foo" and "git checkout foo --"
> >* as candidates for dwim.
> > @@ -1094,6 +1100,9 @@ static int parse_branchname_arg(int argc, const char 
> > **argv,
> >   const char *remote = unique_tracking_name(arg, rev,
> > 
> > dwim_remotes_matched);
> >   if (remote) {
> > + if (disambi_local_file)
> > + die(_("'%s' could be both a local 
> > file and a tracking branch.\n"
> > +   "Please use -- to 
> > disambiguate"), arg);
>
> Ah, the only user of this variable triggers when recover_with_dwim
> is true, so that is how dwim-less case is handled, OK.
>
> That still leaves the question if it is OK to handle these two cases
> the same way in a repository without 'next' branch whose origin has
> one:
>
> $ >next; git checkout --guess next
> $ >next; git checkout --guess 'n??t'
>
> Perhaps the variable should be named "local_file_crashes_with_rev"
> and its the scope narrowed to the same block as "remote" is
> computed.  Or just remove the variable and check the condition right
> there where you need to.  I.e.
>
> if (remote) {
> if (!has_dash_dash &&
> check_filename(opts->prefix, arg))
> die("did you mean a branch or path?: '%s'", arg);
> ...
>

I still think handing both cases the same way is right. In the second
case we would not find 'origin/n??t' so we fall back to checking out
pathspec 'n??t' anyway (expected from you, I think). But just in case
the remote branch 'origin/n??t' exists, we kick and scream.

I think you see 'n??t' as a pathspec, but I'm thinking about a user
who sees 'n??t' as a branch name, not pathspec and he would have a
different expectation.
-- 
Duy

Re: [PATCH/RFC] checkout: print something when checking out paths

[Duy Nguyen]

On Mon, Nov 12, 2018 at 7:21 AM Junio C Hamano  wrote:
>
> Nguyễn Thái Ngọc Duy   writes:
>
> > Since the purpose of printing this is to help disambiguate. Only do it
> > when "--" is missing (the actual reason though is many tests check
> > empty stderr to see that no error is raised and I'm too lazy to fix
> > all the test cases).
>
> Heh, honesty is good but in this particular case the official reason
> alone would make perfect sense, too ;-)
>
> As with progress output, shouldn't this automatically be turned off
> when the standard error stream goes to non tty, as the real purpose
> of printing is to help the user sitting in front of the terminal and
> interacting with the command?

I see this at the same level as "Switched to branch 'foo'" which is
also protected by opts->quiet. If we start hiding messages based on
tty it should be done for other messages in update_refs_for_switch()
too, I guess?

> And by this, I do not mean to say that "When -- is missing" can go
> away.  I agree that "--" is a clear sign that the user knows what
> s/he is doing---to overwrite the paths in the working tree that
> match the pathspec.

My other problem with deciding to print based on the presence of "--"
is also with branch switching code: it always prints something (unless
it actually does nothing). So it's a bit strange that the
checkout_paths() behaves slightly different based on "--".


> > @@ -371,17 +374,27 @@ static int checkout_paths(const struct checkout_opts 
> > *opts,
> >   struct cache_entry *ce = active_cache[pos];
> >   if (ce->ce_flags & CE_MATCHED) {
> >   if (!ce_stage(ce)) {
> > - errs |= checkout_entry(ce, , NULL);
> > + errs |= checkout_entry(ce, ,
> > +NULL, _checkouts);
> >   continue;
>
> As we count inside checkout_entry(), we might not actually write
> this out when we know that the working tree file is up to date
> already but we do not increment in that case either, so we keep
> track of the accurate count of "updates", not path matches (i.e. we
> are not reporting "we made sure this many paths are up to date in
> the working tree"; instead we are reporting how many paths we needed
> to overwrite in the working tree).

Yeah. I counted matched paths first, but when you do "git co .", you
get a huge (and meaningless, to me) number. Printing how many files
are updated makes more sense.

> >   pos = skip_same_name(ce, pos) - 1;
> >   }
> >   }
> > - errs |= finish_delayed_checkout();
> > + errs |= finish_delayed_checkout(, _checkouts);
> > +
> > + if (opts->count_checkout_paths)
> > + fprintf_ln(stderr, Q_("%d path has been updated",
> > +   "%d paths have been updated",
> > +   nr_checkouts),
> > +nr_checkouts);
>
> This one may want to be protected behind isatty(2).  Or the default
> value of count_checkout_paths could be tweakedbased on isatty(2).

Another thing I'm going to change (if this v1 is in the right
direction): print different messages for "git checkout -- foo" and
"git checkout foo -- bar". The first one is "%d paths have been
reverted" but the second one does different things and probably should
say "%d paths have been updated from branch foo" or something like
that.
-- 
Duy

Re: [RFC PATCH] Introduce "precious" file concept

[Duy Nguyen]

On Sun, Nov 11, 2018 at 2:06 PM Ævar Arnfjörð Bjarmason
 wrote:
> +Trashable files
> +~~~
> +
> +`trashable`
> +^^
> +
> +Provides an escape hatch for re-enabling a potentially data destroying
> +feature which was enabled by default between Git versions 1.5.2 and
> +2.20. See the `NOTES` section of linkgit:gitignore[5] for details.

How does this interact with "git clean -x"? Most ignored files will
not have trashable attribute, so we don't remove any of them? Making
"git clean" completely ignore this attribute is also possible, I
guess, if we rename it somehow to avoid confusion.
-- 
Duy

Re: [RFC PATCH] Introduce "precious" file concept

[Duy Nguyen]

On Mon, Nov 12, 2018 at 10:09 AM Matthieu Moy  wrote:
> May I remind an idea I sugested in an old thread: add an intermediate level
> where ignored files to be overwritten are renamed (eg. foo -> foo~ like Emacs'
> backup files):
>
> https://public-inbox.org/git/vpqd3t9656k@bauges.imag.fr/
>
> One advantage of the "rename" behavior is that it's safer that the current,
> but still not very disturbing for people who like the current behavior. This
> makes it a good candidate for a default behavior.

I have something else in the bag that does something like this. The
idea is that we go ahead and do destructive things but we let the user
undo.

Some more background in [1] but basically we hash "every" change and
store in the object database (in this case we store "foo" content
before overwriting it). We maintain a list of these hashes so that
undo is possible, but of course we don't keep infinite change history,
eventually too old changes will be pruned. [1] talks about index
changes (e.g. "git add -p") but it could apply to worktree changes as
well (and I'm also eyeing $GIT_DIR/config changes).

The upside: a similar undo mechanism that works for more than just
this case and it allows undoing multiple times while foo~ only allow
once. The downside: hashing is definitely heavier than renaming foo to
foo~. So this will feature be opt-in in most cases. But for
"dangerous" overwrite like this case, I think we value the file
content more and make it opt-out.

[1] 
https://public-inbox.org/git/CACsJy8A3QCYY6QeJQYkbCKYh=7Q7pj=rer_oqhlgoamqtno...@mail.gmail.com/
-- 
Duy

Re: [RFC PATCH] Introduce "precious" file concept

[Duy Nguyen]

On Sun, Nov 11, 2018 at 1:33 PM Ævar Arnfjörð Bjarmason
 wrote:
> The users who need protection against git deleting their files the most
> are exactly the sort of users who aren't expert-level enough to
> understand the nuances of how the semantics of .gitignore and "precious"
> are going to interact before git eats their data.
>
> This is pretty apparent from the bug reports we're getting about
> this. None of them are:
>
> "Hey, I 100% understood .gitignore semantics including this one part
> of the docs where you say you'll do this, but just forgot one day
> and deleted my work. Can we get some more safety?"
>
> But rather (with some hyperbole for effect):
>
> "ZOMG git deleted my file! Is this a bug??"
>
> So I think we should have the inverse of this "precious"
> attribute". Just a change to the docs to say that .gitignore doesn't
> imply these eager deletion semantics on tree unpacking anymore, and if
> users want it back they can define a "garbage" attribute
> (s/precious/garbage/).
>
> That will lose no data, and in the very rare cases where a checkout of
> tracked files would overwrite an ignored pattern, we can just error out
> (as we do with the "Ok to overwrite" branch removed) and tell the user
> to delete the files to proceed.

There's also the other side of the coin. If this refuse to overwrite
triggers too often, it can become an annoyance. So far I've seen two
reports of accident overwriting which make me think turning precious
to trashable may be too extreme. Plus ignored files are trashable by
default (or at least by design so far), adding trashable attribute
changes how we handle ignored files quite significantly.
-- 
Duy

Re: [PATCH v2 15/16] fsck: reduce word legos to help i18n

[Duy Nguyen]

On Tue, Nov 6, 2018 at 4:41 AM Junio C Hamano  wrote:
> >  static int fsck_error_func(struct fsck_options *o,
> >   struct object *obj, int type, const char *message)
> >  {
> > - objreport(obj, (type == FSCK_WARN) ? "warning" : "error", message);
> > - return (type == FSCK_WARN) ? 0 : 1;
> > + if (type == FSCK_WARN) {
> > + fprintf_ln(stderr, "warning in %s %s: %s",
> > +printable_type(obj), describe_object(obj), 
> > message);
> > + return 0;
> > + }
> > +
> > + fprintf_ln(stderr, "error in %s %s: %s",
> > +printable_type(obj), describe_object(obj), message);
> > + return 1;
>
> Make it look more symmetrical like the original, perhaps by
>
> if (type == FSCK_WARN) {
> ...
> return 0;
> } else { /* FSCK_ERROR */
> ...
> return 1;
> }
>
> Actually, wouldn't it be clearer to see what is going on, if we did
> it like this instead?
>
> const char *fmt = (type == FSCK_WARN)
> ? N_("warning in %s %s: %s")
> : N_("error in %s %s: %s");
> fprintf_ln(stderr, _(fmt),
>printable_type(obj), describe_object(obj), message);
> return (type == FSCK_WARN) ? 0 : 1;
>
> It would show that in either case we show these three things in the
> message.  I dunno.

Specifying "type == FSCK_WARN" twice triggers me (what if we add a
third fsck type?) so I just turn this to a switch/case block instead
(and get to know the third fsck type FSCK_IGNORE).
-- 
Duy

Re: [PATCH v2 12/16] parse-options: replace opterror() with optname()

[Duy Nguyen]

On Tue, Nov 6, 2018 at 3:07 PM Ramsay Jones  wrote:
> Also, this patch does not replace opterror() calls outside of
> the 'parse-options.c' file with optname(). This tickles my
> static-check.pl script, since optname() is an external function
> which is only called from 'parse-options.c'.
>
> So, at present, optname() could be marked as a local 'static'
> symbol. However, I could also imagine it being used by new callers
> outside of 'parse-options.c' in the future. (maybe) Your call. ;-)

I was making it static, but the compiler complained about undefined
function and I would need to either move optname() up in
parse-options.c or add a forward declaration (but I was going to
_remove_ the declaration!)

Since it could be potentially used by Jeff's series (and I think it
does have potential in parse-options-cb.c), I'll just leave it
exported and caress your static-check.pl script (how did it not catch
optbug() not being used outside parse-options.c either)?
-- 
Duy

Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()

[Duy Nguyen]

On Fri, Nov 9, 2018 at 11:19 AM Jeff King  wrote:
> > The form `/abc/def` would not be confused with anything
> > that it is not, I would think. The only thing against this form (at least
> > that I can think of) is that some people use this way to talk about paths
> > that vary between different setups, with the implicit assumption that the
> > reader will "interpolate" this *before* applying. So for example, when I
> > tell a user to please edit their /config, I implicitly assume
> > that they know to not type out, literally,  but instead fill in
> > the correct path.
>
> So yeah, some alternative syntax that is verbose but distinct makes
> sense to me. We use %-substitution elsewhere. Could we do something with
> that? "%RUNTIME_PREFIX%" gives me too many flashbacks, but something
> like "%(RUNTIME_PREFIX)" matches our formatting language.

FWIW I don't have any preference, as long as the variable can still
have a name (that is not a symbol).

A side question regardless of syntax. What do we do with
%(unrecognized name)/foo? I see three options

 - expand to empty, so "/foo"
 - keep it and try the literal path "%(unrecognized name)/foo"
 - somehow tell the caller that the path is invalid and treat it like
non-existing path, even if there is some real thing at "%(unrecognized
name)/foo"

The last option is more predictable. But we need to be more careful
about the syntax because if "%(some path like this)" actually exists
somewhere, then it will be broken. And I think it's also more work.
-- 
Duy

Re: [RFC PATCH] index-pack: improve performance on NFS

[Duy Nguyen]

On Fri, Nov 9, 2018 at 2:46 PM Ævar Arnfjörð Bjarmason  wrote:
> I'm planning to re-submit mine with some minor changes after the great
> Documentation/config* move lands.
>
> As noted in
> https://public-inbox.org/git/87bm7clf4o@evledraar.gmail.com/ and
> https://public-inbox.org/git/87h8gq5zmc@evledraar.gmail.com/ I think
> it's regardless of Jeff's optimization is. O(nothing) is always faster
> than O(something), particularly (as explained in that E-Mail) on NFS.

Is it really worth adding more code to maintain just to shave a couple
seconds (or a few percent clone time)?
-- 
Duy

Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()

[Duy Nguyen]

On Thu, Nov 8, 2018 at 2:14 PM Johannes Schindelin
 wrote:
>
> Hi Peff,
>
> On Wed, 7 Nov 2018, Jeff King wrote:
>
> > All that said, if we're just interested in allowing this for config,
> > then we already have such a wrapper function: git_config_pathname().
>
> Good point. I agree that `git_config_pathname()` is a better home for this
> feature than `expand_user_path()`.
>
> But now I have a really crazy idea: how about ~~/ssl/certs/ca-bundle.crt?
> The `~` prefix is *already* a reserved character, and while it would
> probably not be super intuitive to have `~~` be expanded to the runtime
> prefix, at least it would be less ambiguous than `$RUNTIME_PREFIX` (which
> *is* a valid directory name).

One thing I had in mind when proposing $VARIABLE is that it opens up a
namespace for us to expand more things (*) for example $GIT_DIR (from
~/.gitconfig).

(*) but in a controlled way, it may look like an environment variable,
but we only accept a selected few. And it's only expanded at the
beginning of a path.

> Of course, `~~` is also a valid directory name, but then, so is `~` and we
> do not have any way to specify *that* because `expand_user_path()` will
> always expand it to the home directory. Or am I wrong? Do we have a way to
> disable the expansion?
>
> Ciao,
> Dscho



-- 
Duy

Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert

[Duy Nguyen]

On Tue, Nov 6, 2018 at 11:11 PM Jeff King  wrote:
>
> On Tue, Nov 06, 2018 at 06:48:22PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> > The implementation looks fine to me, but as noted in
> > https://public-inbox.org/git/8736se6qyc@evledraar.gmail.com/ I
> > wonder what the plausible end-game is. That we'll turn this on by
> > default in a few years, and then you can only worry about
> > git-interpret-trailers for repos created as of 2020 or something?
> >
> > Otherwise it seems we'll need to *also* parse out the existing messages
> > we've added.
>
> Could we help the reading scripts by normalizing old and new output via
> interpret-trailers, %(trailers), etc?
>
> I think "(cherry picked from ...)" is already considered a trailer by
> the trailer code. If the caller instructs us to, we could probably
> rewrite it to:
>
>   Cherry-picked-from: ...
>
> in the output. Then the end-game is that scripts should just use
> interpret-trailers, etc, and old and new commits will Just Work.

There is still one thing to settle. "revert -m1" could produce
something like this

This reverts commit , reversing
changes made to .

My proposal produces this

Reverts: ^2

And I can't really convert the former to latter without accessing
object database (probably not a good idea?) to check if SHA2 is the
second parent of SHA1. So either

 - I access object database anyway
 - Generate just "Reverts: " (i.e. losing info) with interpret-trailers
 - Change Reverts: tag to a different output format, or maybe use two
tags instead.
-- 
Duy

Re: What's cooking in git.git (Nov 2018, #03; Wed, 7)

[Duy Nguyen]

On Wed, Nov 7, 2018 at 2:09 PM SZEDER Gábor  wrote:
>
> On Wed, Nov 07, 2018 at 06:41:45PM +0900, Junio C Hamano wrote:
> > * nd/i18n (2018-11-06) 16 commits
> >  - fsck: mark strings for translation
> >  - fsck: reduce word legos to help i18n
> >  - parse-options.c: mark more strings for translation
> >  - parse-options.c: turn some die() to BUG()
> >  - parse-options: replace opterror() with optname()
> >  - repack: mark more strings for translation
> >  - remote.c: mark messages for translation
> >  - remote.c: turn some error() or die() to BUG()
> >  - reflog: mark strings for translation
> >  - read-cache.c: add missing colon separators
> >  - read-cache.c: mark more strings for translation
> >  - read-cache.c: turn die("internal error") to BUG()
> >  - attr.c: mark more string for translation
> >  - archive.c: mark more strings for translation
> >  - alias.c: mark split_cmdline_strerror() strings for translation
> >  - git.c: mark more strings for translation
> >
> >  More _("i18n") markings.
>
> When this patch is merged into 'pu' all four tests added to
> 't1450-fsck.sh' in b29759d89a (fsck: check HEAD and reflog from other
> worktrees, 2018-10-21) as part of 'nd/per-worktree-ref-iteration'
> below fail when run with GETTEXT_POISON=y.  The test suite passes in
> both of these topics on their own, even with GETTEXT_POISON, it's
> their merge that is somehow problematic.

Not surprising. The i18n series makes fsck output localized strings
and without updating grep to test_i18ngrep, new tests will fail. If
'pu' was passing before, I'm ok with just ejecting this series for
now. Then I wait for the other to land, rebase, fixup and resubmit.
-- 
Duy

Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()

[Duy Nguyen]

On Tue, Nov 6, 2018 at 7:15 PM Ramsay Jones  wrote:
> >> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int 
> >> real_home)
> >>
> >>  if (path == NULL)
> >>  goto return_null;
> >> +#ifdef __MINGW32__
> >> +if (path[0] == '/')
> >> +return system_path(path + 1);
> >> +#endif
> >
> > Hmm, this doesn't quite fit with the intended use of this
> > function! ;-) (even on windows!)
> >
> > I haven't looked very deeply, but doesn't this affect all
> > absolute paths in the config read by git_config_pathname(),
> > along with all 'included config' files?
> >
> > I am pretty sure that I would not want the absolute paths
> > in my config file(s) magically 'moved' depending on whether
> > git has been compiled with 'runtime prefix' support or not!
>
> So, I hit 'send' before finishing my thought ...
>
> I can't think of a non-backwards compatible way of doing
> what you want. If backward compatibility wasn't an issue,
> then we could (maybe) have used some kind of pathname prefix
> like 'system:/path/relative/to/git/executable', or somesuch.

A pseudo variable might work, like $ROOT/path/relative/to/somewhere
-- 
Duy

Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()

[Duy Nguyen]

On Tue, Nov 6, 2018 at 3:55 PM Johannes Schindelin via GitGitGadget
 wrote:
>
> From: Johannes Schindelin 
>
> On Windows, an absolute POSIX path needs to be turned into a Windows
> one.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  path.c | 5 +
>  1 file changed, 5 insertions(+)
>
> diff --git a/path.c b/path.c
> index 34f0f98349..a72abf0e1f 100644
> --- a/path.c
> +++ b/path.c
> @@ -11,6 +11,7 @@
>  #include "path.h"
>  #include "packfile.h"
>  #include "object-store.h"
> +#include "exec-cmd.h"
>
>  static int get_st_mode_bits(const char *path, int *mode)
>  {
> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
>
> if (path == NULL)
> goto return_null;
> +#ifdef __MINGW32__
> +   if (path[0] == '/')
> +   return system_path(path + 1);
> +#endif

Should this behavior be documented somewhere, maybe in config.txt?

> if (path[0] == '~') {
> const char *first_slash = strchrnul(path, '/');
> const char *username = path + 1;
> --
> gitgitgadget
-- 
Duy

Re: [PATCH] format-patch: respect --stat when explicitly specified

[Duy Nguyen]

On Tue, Nov 6, 2018 at 6:25 PM Leif Lindholm  wrote:
> > > Sure. Only today was the first time I had a look at the git sources,
> > > so some guidance would be most appreciated.
> >
> > No problem (and if you don't have time to do it, just say the word and
> > I will continue; this is my bug after all)
>
> Wll, if you're offering, I would certainly appreciate not having
> to dig deeper into this. I've got a patch review backlog the length of
> my arm in another project...

Your loss. Your name is not going to be in git.git then (j/k). Thanks
for the report. I'll double, triple check, add tests and resubmit
soon.
-- 
Duy

Re: [PATCH] format-patch: respect --stat when explicitly specified

[Duy Nguyen]

On Tue, Nov 6, 2018 at 5:31 PM Leif Lindholm  wrote:
> > > diff --git a/builtin/log.c b/builtin/log.c
> > > index 061d4fd86..07e6ae2c1 100644
> > > --- a/builtin/log.c
> > > +++ b/builtin/log.c
> > > @@ -1009,7 +1009,8 @@ static void show_diffstat(struct rev_info *rev,
> > >
> > > memcpy(, >diffopt, sizeof(opts));
> > > opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> > > -   opts.stat_width = MAIL_DEFAULT_WRAP;
> > > +   if (rev->diffopt.stat_width == -1)
> >
> > I don't think we get -1 here when stat_width is not defined. The
> > "undefined" value is zero but I'm pretty sure we get MAIL_DEFAULT_WRAP
> > in here unless you specify --stat.
>
> From what I could tell, if nothing is specified on command line,
> rev->diffopt.stat_width is set to -1 at this point (I assumed by
> rev->cmd_log_init_defaults(), but didn't go digging).

I thought the same but could find where cmd_log_.. is called by
format-patch. I was not even sure if I read the code correctly so I
ran the command through gdb. It was definitely not called.

> The patched version certainly gives the <= 2.16.* behaviour with
> --stat and still restricts stat lines to 72 characters without.
>
> > So I think you can just drop the below assignment. But if you want to
> > be on the safe side, check for zero stat_width instead of -1 and set
> > MAIL_DEFAULT_WRAP.
> >
> > > +   opts.stat_width = MAIL_DEFAULT_WRAP;
> >
> > How about a test to make sure this will not be broken in future?
>
> Sure. Only today was the first time I had a look at the git sources,
> so some guidance would be most appreciated.

No problem (and if you don't have time to do it, just say the word and
I will continue; this is my bug after all)

> Should I add a function to t/t4014-format-patch.sh and just put
> something longer than a/file for the expect template?

First of all the README file in that directory could give pretty good
basic instructions.

Back to this test, I think you could start by copying to a new test
(the whole "test_expect_success" block, optionally including the
"expect" file creation too), add --stat there and see what it looks
like. For stat testing, t4052 could also be a good example. Or perhaps
the test should be added in t4052 because it already supports long
file name ($name is 120 char long).
-- 
Duy

Re: [PATCH] format-patch: respect --stat when explicitly specified

[Duy Nguyen]

On Tue, Nov 6, 2018 at 5:18 PM Laszlo Ersek  wrote:
> >> +   opts.stat_width = MAIL_DEFAULT_WRAP;
> >
> > How about a test to make sure this will not be broken in future?
>
> Oh, looks like I won't have to test this patch at all! ;)
>
> (Just kidding, I'll test the next iteration.)

Just to be clear I didn't mean you didn't test it. I meant adding a
new test case to our test suite.
-- 
Duy

Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines

[Duy Nguyen]

On Tue, Nov 6, 2018 at 9:57 AM Ævar Arnfjörð Bjarmason  wrote:
> Leaving aside the question of whether the pain of switching is worth it,
> I think it's a worthwihle to consider if we could stop hardcoding one
> specific human language in commit messages, and instead leave something
> machine-readable behind.
>
> We do that with reverts, and also with merge commits, which could be
> given a similar treatment where we change e.g. "Merge branches
> 'jc/convert', 'jc/bigfile' and 'jc/replacing' into jc/streaming" (to use
> git.git's 02071b27f1 as an example) to:
>
> Merge-branch-1: jc/convert
> Merge-branch-2: jc/bigfile
> Merge-branch-3: jc/replacing
> Merge-branch-into: jc/streaming
>
> Then, when rendering the commit in the UI we could parse that out, and
> put a "Merge branches[...]" message at the top, except this time in the
> user's own language.

My first reaction of this was "but branch name is a local thing and
not significant anyway!". But then if people use one branch as a
bundle of other branches like 'pu', then the ability to recreate
branches (with the right name of course) from those merges may be
useful. So... yeah, maybe.
-- 
Duy

Re: Understanding pack format

[Duy Nguyen]

On Tue, Nov 6, 2018 at 3:23 AM Farhan Khan  wrote:
> To follow-up from the other day, I have been reading the code that
> retrieves the pack entry for the past 3 days now without much success.
> But there are quite a few abstractions and I get lost half-way down
> the line.

Jeff already gave you some pointers. This is just a side note.

I think it's easier to run the code under a debugger and see what it
does than just reading it. You can create a repo with just one blob to
have better control over it (small packs also make it possible to
examine with a hex editor in parallel), e.g.

git init foo
cd foo
echo hello >file
git add file
git repack -ad
gdb --args git show :./file

then put a breakpoint in some interesting functions (perhaps one of
those Jeff pointed out)
-- 
Duy

Re: [PATCH] format-patch: respect --stat when explicitly specified

[Duy Nguyen]

On Tue, Nov 6, 2018 at 11:48 AM Leif Lindholm  wrote:
>
> Commit 43662b23abbd
> ("format-patch: keep cover-letter diffstat wrapped in 72 columns") made
> format-patch keep the diffstat to within 72 characters. However, it does
> this even when --stat is explicitly set on the command line.
>
> Make it possible to explicitly override the new mechanism, using --stat,
> matching the functionality before this change. This also matches the
> output in the case of non-cover-letter files.
>
> Cc: Nguyễn Thái Ngọc Duy 
> Cc: Junio C Hamano 
> Reported-by: Laszlo Ersek 
> Signed-off-by: Leif Lindholm 
> ---
>
> Note:
> In TianoCore we have LotsOfGloriousFilesNamedInReallyLongCamelCase, so
> our official submission guidelines specify the use of --stat.
>
>  builtin/log.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 061d4fd86..07e6ae2c1 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1009,7 +1009,8 @@ static void show_diffstat(struct rev_info *rev,
>
> memcpy(, >diffopt, sizeof(opts));
> opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> -   opts.stat_width = MAIL_DEFAULT_WRAP;
> +   if (rev->diffopt.stat_width == -1)

I don't think we get -1 here when stat_width is not defined. The
"undefined" value is zero but I'm pretty sure we get MAIL_DEFAULT_WRAP
in here unless you specify --stat.

So I think you can just drop the below assignment. But if you want to
be on the safe side, check for zero stat_width instead of -1 and set
MAIL_DEFAULT_WRAP.

> +   opts.stat_width = MAIL_DEFAULT_WRAP;

How about a test to make sure this will not be broken in future?

>
> diff_setup_done();
>
> --
> 2.11.0
-- 
Duy

Re: [PATCH v1] refresh_index: remove unnecessary calls to preload_index()

[Duy Nguyen]

On Mon, Nov 5, 2018 at 8:30 PM Ben Peart  wrote:
>
> From: Ben Peart 
>
> With refresh_index() learning to utilize preload_index() to speed up its
> operation there is no longer any benefit to having the caller preload the
> index first. Remove those unneeded calls by calling read_index() instead of
> the preload variant.
>
> There is no measurable performance impact of this patch - the 2nd call to
> preload_index() bails out quickly but there is no reason to call it twice.

Obviously correct. It's not shown in the context lines, but there's
also a refresh_index() after read_index() in sequencer.c too.
-- 
Duy

Re: Design of multiple hash support

[Duy Nguyen]

On Mon, Nov 5, 2018 at 2:02 AM brian m. carlson
 wrote:
>
> I'm currently working on getting Git to support multiple hash algorithms
> in the same binary (SHA-1 and SHA-256).  In order to have a fully
> functional binary, we'll need to have some way of indicating to certain
> commands (such as init and show-index) that they should assume a certain
> hash algorithm.
>
> There are basically two approaches I can take.  The first is to provide
> each command that needs to learn about this with its own --hash
> argument.  So we'd have:
>
>   git init --hash=sha256
>   git show-index --hash=sha256 
> The other alternative is that we provide a global option to git, which
> is parsed by all programs, like so:
>
>   git --hash=sha256 init
>   git --hash=sha256 show-index 

I suppose this is about the "no repository/standalone" mode, because

 - it's hard to pass global arguments down to builtin commands (we
often have to rely on global variables which are on the way out)

 - global options confuse new people and also harder to reorder (if
you forget it, you have to alt-b all the way back to near the
beginning of the command line and add it there, instead of near the
end)

 - there aren't that many standalone commands

I'm leaning towards "git foo --hash=".

> There's also the question of what we want to call the option.  The
> obvious name is --hash, which is intuitive and straightforward.
> However, the transition plan names the config option
> extensions.objectFormat, so --object-format is also a possibility.  If
> we ever decide to support, say, zstd compression instead of zlib, we
> could leverage the same option (say, --object-format=sha256:zstd) and
> avoid the need for an additional option.  This might be planning for a
> future that never occurs, though.

--object-format is less vague than --hash. The downside is it's longer
(more to type) but I'm counting on git-completion.bash and the guess
that people rarely need to use this option.
-- 
Duy

Re: [PATCH 0/13] parseopt fixes from -Wunused-parameters

[Duy Nguyen]

On Mon, Nov 5, 2018 at 7:49 PM Jeff King  wrote:
>
> On Mon, Nov 05, 2018 at 05:51:07PM +0100, Duy Nguyen wrote:
>
> > On Mon, Nov 5, 2018 at 7:39 AM Jeff King  wrote:
> > >
> > > Continuing my exploration of what -Wunused-parameters can show us, here
> > > are some bug-fixes related to parse-options callbacks.
> > >
> > > This is the last of the actual bug-fixes I've found. After this, I have
> > > about 60 patches worth of cleanups (i.e., dropping the unused
> > > parameters), and then I have a series to annotate parameters that must
> > > be unused (e.g., for functions that must conform to callback
> > > interfaces). After we can start compiling with -Wunused-parameters,
> > > assuming we don't find the annotations too cumbersome.
> >
> > Another good thing from this series is there are fewer --no-options to 
> > complete.
> >
> > About the annotating unused parameters related to segfault bug fixes
> > in this series. Should we just add something like
> >
> >  if (unset)
> > BUG("This code does not support --no-stuff");
> >
> > which could be done in the same patches that fix the segfault, and it
> > extends the diff context a bit to see what these callbacks do without
> > opening up the editor, and also serves as a kind of annotation?
>
> That's done in the final patch. I did originally do it alongside the
> actual segfault fixes, but since it needs doing in so many other
> callbacks, too, it made sense to me to do it all together.

Oops. I guess I stopped reading the series too early. Sorry for the noise.
-- 
Duy

Re: [PATCH 12/12] fsck: mark strings for translation

[Duy Nguyen]

On Mon, Oct 29, 2018 at 12:53 PM SZEDER Gábor  wrote:
> The contents of 'out':
>
>   broken link fromtree be45bbd3809e0829297cefa576e699c134abacfd 
> (refs/heads/master@{1112912113}:caesar.t)
> toblob be45bbd3809e0829297cefa576e699c134abacfd 
> (refs/heads/master@{1112912113}:caesar.t)
>   missing blob be45bbd3809e0829297cefa576e699c134abacfd 
> (refs/heads/master@{1112912113}:caesar.t)
>
> On a related (side)note, I think it would be better if this 'grep'

I found the problem. Some function returns static string which works
fine when we do two printf()'s, one call of that function per
printf(). But when combining two printf() in one, we have a problem.
Thanks for catching.
-- 
Duy

Re: [PATCH 0/13] parseopt fixes from -Wunused-parameters

[Duy Nguyen]

On Mon, Nov 5, 2018 at 7:39 AM Jeff King  wrote:
>
> Continuing my exploration of what -Wunused-parameters can show us, here
> are some bug-fixes related to parse-options callbacks.
>
> This is the last of the actual bug-fixes I've found. After this, I have
> about 60 patches worth of cleanups (i.e., dropping the unused
> parameters), and then I have a series to annotate parameters that must
> be unused (e.g., for functions that must conform to callback
> interfaces). After we can start compiling with -Wunused-parameters,
> assuming we don't find the annotations too cumbersome.

Another good thing from this series is there are fewer --no-options to complete.

About the annotating unused parameters related to segfault bug fixes
in this series. Should we just add something like

 if (unset)
BUG("This code does not support --no-stuff");

which could be done in the same patches that fix the segfault, and it
extends the diff context a bit to see what these callbacks do without
opening up the editor, and also serves as a kind of annotation?
-- 
Duy

Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines

[Duy Nguyen]

On Mon, Nov 5, 2018 at 1:56 AM Junio C Hamano  wrote:
>
> Nguyễn Thái Ngọc Duy   writes:
>
> > A reverted commit will have a new trailer
> >
> > Revert: 
>
> Please don't, unless you are keeping the current "the effect of
> commit X relative to its parent Y was reverted" writtein in prose,
> which is meant to be followed up with a manually written "because
> ..." and adding this as an extra footer that is meant solely for
> machine consumption.  Of course reversion of a merge needs to say
> relative to which parent of the merge it is undoing.

I think the intent of writing "This reverts  " to encourage
writing "because ..." is good, but in practice many people just simply
not do it. And by not describing anything at all (footers don't count)
some commit hook can force people to actually write something.

But for the transition period I think we need to keep both anyway,
whether this "This reverts ..." should stay could be revisited another
day (or not, even).

> > Similarly a cherry-picked commit with -x will have
> >
> > Cherry-Pick: 
>
> Unlike the "revert" change above, this probably is a good change, as
> a"(cherry-pickt-from: X)" does not try to convey anything more or
> anything less than such a standard looking trailer and it is in
> different shape only by historical accident.  But people's scripts
> may need to have a long transition period for this change to happen.

Yep. I'll code something up to print both by default with config knobs
to disable either. Unless you have some better plan of course.
-- 
Duy

Re: [PATCH/RFC v2] sequencer.c: record revert/cherry-pick commit with trailer lines

[Duy Nguyen]

On Sun, Nov 4, 2018 at 10:30 PM brian m. carlson
 wrote:
> However, I do have concerns about breaking compatibility with existing
> scripts.  I wonder if we could add a long alias for git cherry-pick -x,
> say "--notate" and have "--notate=text" mean "-x" and "--notate=trailer"
> mean this new format.  Similarly, git revert could learn such an option
> as well.

I don't think it will help unless you are the only developer on some
repo. If you have some scripts parsing the old format, people could
choose to commit using the new format anyway and your scripts will
have to adapt (it's too late to revert because it's already part of
git history).

The transition plan could be outputing both old and new formats at the
same time (optionally allowing to disable the old one) and leave it
like that for a couple releases. Then we could stop producing the old
output and hope that all the scripts in the world have caught up. Not
a great plan.

> One final thought: since our trailers seem to act as if we wrote "this
> commit" (has been), I wonder if we should say "Reverts" instead of
> "Revert" for consistency.

Yeah that or Reverting:, both are better than Revert:. I guess I'll
just go with Reverts: if this patch moves forward.
-- 
Duy

Re: "git checkout" safety feature

[Duy Nguyen]

On Mon, Nov 5, 2018 at 7:53 AM Jeff King  wrote:
>
> On Mon, Nov 05, 2018 at 07:24:42AM +0100, Matthias Urlichs wrote:
>
> > Hi,
> > > "git checkout  " is a feature to overwrite local
> > > changes.  It is what you use when you make a mess editing the files
> > > and want to go back to a known state.  Why should that feature be
> > > destroyed?
> >
> > Not destroyed, but optionally made finger-fumble-save – like "alias rm
> > rm -i".
>
> There are a couple of destructive commands left in Git (e.g., this one,
> and "git reset --hard" is another). I didn't dig up archive references,
> but the topic of safety valves has come up many times over the years.
> The discussion usually ends with the notion that instead of warning
> that the operation is destructive (because that gets annoying when its
> purpose is to be destructive), we should make it possible to undo a
> mistake.
>
> So in this case, that would mean saving the working tree file to a blob
> before we obliterate it.
>
> See similar discussion in:
>
>   
> https://public-inbox.org/git/cacsjy8c5qolvg4pzy_pthqoygh9ohdevhxsuywqhqypn3ob...@mail.gmail.com/
>
> for example.

That work is still ongoing (slowly). I realized that reflog code was
buried deep in files-backend.c and would not make sense to reuse in
its current form. So I had to move the code to a common place, which
adds more work. But it will be coming! Hopefully before 2020 at my
usual development speed.

While we're at it, I've been running something with that "index
reflog" (no pruning) for a month with lots of "add -p" and the file
size is just 163KB, so the reflog format seems promising for this
purpose.
-- 
Duy

Re: [RFC v1] Add virtual file system settings and hook proc

[Duy Nguyen]

On Mon, Nov 5, 2018 at 12:40 PM Ævar Arnfjörð Bjarmason
 wrote:
>
>
> On Sun, Nov 04 2018, Duy Nguyen wrote:
>
> > On Wed, Oct 31, 2018 at 9:53 PM Ben Peart  wrote:
> >> >> +core.virtualFilesystem::
> >> >> +   If set, the value of this variable is used as a command which
> >> >> +   will identify all files and directories that are present in
> >> >> +   the working directory.  Git will only track and update files
> >> >> +   listed in the virtual file system.  Using the virtual file 
> >> >> system
> >> >> +   will supersede the sparse-checkout settings which will be 
> >> >> ignored.
> >> >> +   See the "virtual file system" section of linkgit:githooks[6].
> >> >
> >> > It sounds like "virtual file system" is just one of the use cases for
> >> > this feature, which is more about a dynamic source of sparse-checkout
> >> > bits. Perhaps name the config key with something along sparse checkout
> >> > instead of naming it after a use case.
> >>
> >> It's more than a dynamic sparse-checkout because the same list is also
> >> used to exclude any file/folder not listed.  That means any file not
> >> listed won't ever be updated by git (like in 'checkout' for example) so
> >> 'stale' files could be left in the working directory.  It also means git
> >> won't find new/untracked files unless they are specifically added to the
> >> list.
> >
> > OK. I'm not at all interested in carrying maintenance burden for some
> > software behind closed doors. I could see values in having a more
> > flexible sparse checkout but this now seems like very tightly designed
> > for GVFS. So unless there's another use case (preferably open source)
> >  for this, I don't think this should be added in git.git.
>
> I haven't looked at the patch in any detail beyond skimming it, and
> you're more familiar with this area...
>
> But in principle I'm very interested in getting something like this in
> git.git, even if we were to assume GVFS was a 100% proprietary
> implementation.

I have nothing against building a GVFS-like solution. If what's
submitted can be the building blocks for that, great! But if it was
just for GVFS (and it was not available to everybody) then no thank
you.
-- 
Duy

Re: [RFC v1] Add virtual file system settings and hook proc

[Duy Nguyen]

On Sun, Nov 4, 2018 at 10:01 PM brian m. carlson
 wrote:
>
> On Sun, Nov 04, 2018 at 07:34:01AM +0100, Duy Nguyen wrote:
> > On Wed, Oct 31, 2018 at 9:53 PM Ben Peart  wrote:
> > > It's more than a dynamic sparse-checkout because the same list is also
> > > used to exclude any file/folder not listed.  That means any file not
> > > listed won't ever be updated by git (like in 'checkout' for example) so
> > > 'stale' files could be left in the working directory.  It also means git
> > > won't find new/untracked files unless they are specifically added to the
> > > list.
> >
> > OK. I'm not at all interested in carrying maintenance burden for some
> > software behind closed doors. I could see values in having a more
> > flexible sparse checkout but this now seems like very tightly designed
> > for GVFS. So unless there's another use case (preferably open source)
> > for this, I don't think this should be added in git.git.
>
> I should point out that VFS for Git is an open-source project and will
> likely have larger use than just at Microsoft.  There are both Windows
> and Mac clients and there are plans for a Linux client as well.
> Ideally, it would work with an unmodified upstream Git, which is (I
> assume) why Ben is sending this series.

Ah I didn't know that. Thank you. I'll have to look at this GVFS some time then.

If we're going to support GVFS though, I think there should be a big
(RFC perhaps) series that includes everything to at least give an
overview what the end game looks like. Then it could be split up into
smaller series.
-- 
Duy

Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines

[Duy Nguyen]

On Sun, Nov 4, 2018 at 5:45 PM Phillip Wood  wrote:
>
> On 04/11/2018 07:22, Nguyễn Thái Ngọc Duy wrote:
> > When a commit is reverted (or cherry-picked with -x) we add an English
> > sentence recording that commit id in the new commit message. Make
> > these real trailer lines instead so that they are more friendly to
> > parsers (especially "git interpret-trailers").
> >
> > A reverted commit will have a new trailer
> >
> >  Revert: 
> >
> > Similarly a cherry-picked commit with -x will have
> >
> >  Cherry-Pick: 
>
> I think this is a good idea though I wonder if it will break someones
> script that is looking for the messages generated by -x at the moment.

It will [1] but I still think it's worth the trouble. The script will
be less likely to break after, and you can use git-interpret-trailers
instead of plain grep.

[1] 
https://public-inbox.org/git/20181017143921.gr270...@devbig004.ftw2.facebook.com/

> > @@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command 
> > command, struct commit *commit,
> >   base_label = msg.label;
> >   next = parent;
> >   next_label = msg.parent_label;
> > - strbuf_addstr(, "Revert \"");
> > - strbuf_addstr(, msg.subject);
> > - strbuf_addstr(, "\"\n\nThis reverts commit ");
> > - strbuf_addstr(, oid_to_hex(>object.oid));
> > -
> > - if (commit->parents && commit->parents->next) {
> > - strbuf_addstr(, ", reversing\nchanges made to 
> > ");
> > - strbuf_addstr(, 
> > oid_to_hex(>object.oid));
> > - }
>
> As revert currently records the parent given on the command line when
> reverting a merge commit it would probably be a good idea to add that
> either as a separate trailer or to the Revert: trailer and possibly also
> generate it for cherry picks.

My mistake. I didn't read carefully and thought it was logging
commit->parents, which is pointless.

So what should be the trailer for this (I don't think putting it in
Revert: is a good idea, too much to parse)? Revert-parent: ?
Revert-merge: ?

> > - strbuf_addstr(, ".\n");
> > + strbuf_addf(, "Revert \"%s\"\n\n", msg.subject);
>
> If the message already contains trailers then should we just append the
> Revert trailer those rather than inserting "\n\n"?

Umm.. but this \n\n is for separating the subject and the body. I
think we need it anyway, trailer or not.

> > @@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command command, 
> > struct commit *commit,
> >   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");
> > + strbuf_addf(, "Cherry-Pick: %s\n",
> > + oid_to_hex(>object.oid));

I will probably make this "Cherry-picked-from:" to match our S-o-b style.
-- 
Duy

Re: [RFC v1] Add virtual file system settings and hook proc

[Duy Nguyen]

On Wed, Oct 31, 2018 at 9:53 PM Ben Peart  wrote:
> >> +core.virtualFilesystem::
> >> +   If set, the value of this variable is used as a command which
> >> +   will identify all files and directories that are present in
> >> +   the working directory.  Git will only track and update files
> >> +   listed in the virtual file system.  Using the virtual file system
> >> +   will supersede the sparse-checkout settings which will be ignored.
> >> +   See the "virtual file system" section of linkgit:githooks[6].
> >
> > It sounds like "virtual file system" is just one of the use cases for
> > this feature, which is more about a dynamic source of sparse-checkout
> > bits. Perhaps name the config key with something along sparse checkout
> > instead of naming it after a use case.
>
> It's more than a dynamic sparse-checkout because the same list is also
> used to exclude any file/folder not listed.  That means any file not
> listed won't ever be updated by git (like in 'checkout' for example) so
> 'stale' files could be left in the working directory.  It also means git
> won't find new/untracked files unless they are specifically added to the
> list.

OK. I'm not at all interested in carrying maintenance burden for some
software behind closed doors. I could see values in having a more
flexible sparse checkout but this now seems like very tightly designed
for GVFS. So unless there's another use case (preferably open source)
for this, I don't think this should be added in git.git.
-- 
Duy

Re: [PATCH] tree-walk.c: fix overoptimistic inclusion in :(exclude) matching

[Duy Nguyen]

On Sun, Nov 4, 2018 at 7:27 AM Eric Sunshine  wrote:
>
> On Sat, Nov 3, 2018 at 8:25 PM Eric Sunshine  wrote:
> > On Sat, Nov 3, 2018 at 11:31 AM Nguyễn Thái Ngọc Duy  
> > wrote:
> > > +test_expect_success 't_e_i() exclude case #8' '
> > > +   git init case8 &&
> > > +   (
> > > +   cd case8 &&
> > > +   echo file >file1 &&
> > > +   echo file >file2 &&
> > > +   git add . &&
> >
> > Won't this loose git-add invocation end up adding all the junk files
> > from earlier tests? One might have expected to see the more restricted
> > invocation:
> > git add file1 file2 &&
> > to make it easier to reason about the test and not worry about someone
> > later inserting tests above this one which might interfere with it.
>
> Upon reflection, there shouldn't be any junk files since this test is
> creating a new repository and "file1" and "file2" are the only files
> present. Apparently, I wasn't paying close enough attention when I
> made the earlier observation.

Yup.

> Anyhow, the more restricted git-add invocation you used in the re-roll
> is still preferable since it makes the intention obvious. Thanks.

Which is why I did it anyway :)
-- 
Duy

Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits

[Duy Nguyen]

On Sat, Nov 3, 2018 at 4:32 PM Michał Górny  wrote:
> Perhaps this is indeed specific to this version of GnuPG.  The tests
> pass for me with both 1.4.21 and 2.2.10.  We don't have 2.1* in Gentoo
> anymore.

Updated to 2.2.8 and the test is passed.
-- 
Duy

Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits

[Duy Nguyen]

On Sat, Nov 3, 2018 at 4:32 PM Michał Górny  wrote:
> > Perhaps my gpg is too old?
> >
> > $ gpg --version
> > gpg (GnuPG) 2.1.15
> > libgcrypt 1.7.3
> > Copyright (C) 2016 Free Software Foundation, Inc.
> > License GPLv3+: GNU GPL version 3 or later 
> > 
> > This is free software: you are free to change and redistribute it.
> > There is NO WARRANTY, to the extent permitted by law.
> >
> > Home: /home/pclouds/.gnupg
> > Supported algorithms:
> > Pubkey: RSA, ELG, DSA, ECDH, ECDSA, EDDSA
> > Cipher: IDEA, 3DES, CAST5, BLOWFISH, AES, AES192, AES256, TWOFISH,
> > CAMELLIA128, CAMELLIA192, CAMELLIA256
> > Hash: SHA1, RIPEMD160, SHA256, SHA384, SHA512, SHA224
> > Compression: Uncompressed, ZIP, ZLIB, BZIP2
>
> Perhaps this is indeed specific to this version of GnuPG.  The tests
> pass for me with both 1.4.21 and 2.2.10.  We don't have 2.1* in Gentoo
> anymore.

Yeah I have not really used gpg and neglected updating it. Will try it
now. The question remains though whether we need to support 2.1* (I
don't know at all about gnupg status, maybe 2.1* is indeed too
old/buggy that nobody should use it and so we don't need to support
it).
-- 
Duy

Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits

[Duy Nguyen]

On Sat, Oct 20, 2018 at 9:31 PM Michał Górny  wrote:
> +test_expect_success GPG 'detect fudged commit with double signature' '
> +   sed -e "/gpgsig/,/END PGP/d" forged1 >double-base &&
> +   sed -n -e "/gpgsig/,/END PGP/p" forged1 | \
> +   sed -e "s/^gpgsig//;s/^ //" | gpg --dearmor >double-sig1.sig 
> &&
> +   gpg -o double-sig2.sig -u 29472784 --detach-sign double-base &&
> +   cat double-sig1.sig double-sig2.sig | gpg --enarmor 
> >double-combined.asc &&
> +   sed -e "s/^\(-.*\)ARMORED FILE/\1SIGNATURE/;1s/^/gpgsig /;2,\$s/^/ /" 
> \
> +   double-combined.asc > double-gpgsig &&
> +   sed -e "/committer/r double-gpgsig" double-base >double-commit &&
> +   git hash-object -w -t commit double-commit >double-commit.commit &&
> +   test_must_fail git verify-commit $(cat double-commit.commit) &&
> +   git show --pretty=short --show-signature $(cat double-commit.commit) 
> >double-actual &&
> +   grep "BAD signature from" double-actual &&
> +   grep "Good signature from" double-actual
> +'

This test fails on 'master' today for me

gpg: WARNING: multiple signatures detected.  Only the first will be checked.
gpg: Signature made Sat Nov  3 15:13:28 2018 UTC
gpg:using DSA key 13B6F51ECDDE430D
gpg:issuer "commit...@example.com"
gpg: BAD signature from "C O Mitter " [ultimate]
gpg: BAD signature from "C O Mitter " [ultimate]
not ok 16 - detect fudged commit with double signature

Perhaps my gpg is too old?

$ gpg --version
gpg (GnuPG) 2.1.15
libgcrypt 1.7.3
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later 
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Home: /home/pclouds/.gnupg
Supported algorithms:
Pubkey: RSA, ELG, DSA, ECDH, ECDSA, EDDSA
Cipher: IDEA, 3DES, CAST5, BLOWFISH, AES, AES192, AES256, TWOFISH,
CAMELLIA128, CAMELLIA192, CAMELLIA256
Hash: SHA1, RIPEMD160, SHA256, SHA384, SHA512, SHA224
Compression: Uncompressed, ZIP, ZLIB, BZIP2
-- 
Duy

Re: [[PATCH v2]] commit: add a commit.allowempty config variable

[Duy Nguyen]

On Sat, Nov 3, 2018 at 12:55 PM tanushree27  wrote:
>
> Add commit.allowempty configuration variable as a convenience for those
> who always prefer --allow-empty.
>
> Add tests to check the behavior introduced by this commit.
>
> This closes https://github.com/git-for-windows/git/issues/1854
>
> Signed-off-by: tanushree27 
> ---
>  Documentation/config.txt |  5 +
>  Documentation/git-commit.txt |  3 ++-
>  builtin/commit.c |  8 
>  t/t7500-commit.sh| 32 
>  4 files changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index c0727b7866..ac63b12ab3 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1467,6 +1467,11 @@ commit.verbose::
> A boolean or int to specify the level of verbose with `git commit`.
> See linkgit:git-commit[1].
>
> +commit.allowempty::

The current naming convention is camelCase. So this should be commit.allowEmpty.

> +   A boolean to specify whether empty commits are allowed with `git
> +   commit`. See linkgit:git-commit[1].
> +   Defaults to false.
> +
>  credential.helper::
> Specify an external helper to be called when a username or
> password credential is needed; the helper may consult external
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index f970a43422..07a5b60ab9 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -176,7 +176,8 @@ The `-m` option is mutually exclusive with `-c`, `-C`, 
> and `-F`.
> Usually recording a commit that has the exact same tree as its
> sole parent commit is a mistake, and the command prevents you
> from making such a commit.  This option bypasses the safety, and
> -   is primarily for use by foreign SCM interface scripts.
> +   is primarily for use by foreign SCM interface scripts. See
> +   `commit.allowempty` in linkgit:git-config[1].

Same.

>
>  --allow-empty-message::
> Like --allow-empty this command is primarily for use by foreign

-- 
Duy

Re: [PATCH v2] completion: use builtin completion for format-patch

[Duy Nguyen]

On Sat, Nov 3, 2018 at 8:59 AM Denton Liu  wrote:
> > @@ -2081,7 +2077,7 @@ _git_send_email ()
> >   return
> >   ;;
> >   --*)
> > - __gitcomp "--annotate --bcc --cc --cc-cmd --chain-reply-to
> > + __gitcomp_builtin send-email "--annotate --bcc --cc --cc-cmd 
> > --chain-reply-to
> >   --compose --confirm= --dry-run --envelope-sender
> >   --from --identity
> >   --in-reply-to --no-chain-reply-to 
> > --no-signed-off-by-cc
>
> Would it make sense to make send-email's completion helper print these
> out directly? That way, if someone were to modify send-email in the
> future, they'd only have to look through one file instead of both
> send-email and the completions script.

I did think about that and decided not to do it (in fact the first
revision of this patch did exactly that).

If we have to maintain this list manually, we might as well leave to
the place that matters: the completion script. I don't think the
person who updates send-email.perl would be always interested in
completion support. And the one that is interested usually only looks
at the completion script. Putting this list in send-email.perl just
makes it harder to find.
-- 
Duy

Re: [PATCH v2] completion: use builtin completion for format-patch

[Duy Nguyen]

On Fri, Nov 02, 2018 at 08:52:30AM +0900, Junio C Hamano wrote:
> Duy Nguyen  writes:
> 
> >> > I have no comment about this. In an ideal world, sendemail.perl could
> >> > be taught to support --git-completion-helper but I don't think my
> >> > little remaining Perl knowledge (or time) is enough to do it. Perhaps
> >> > this will do. I don't know.
> >>
> >> So "all", "attach", etc. are added to this list while these similar
> >> options are lost from the other variable?  Is this a good trade-off?
> >
> > Not sure if I understand you correctly, but it looks to me that the
> > options in git-send-email.perl are well organized, so we could...
> 
> Yes, but I wasn't commenting on your "sendemail should also be able
> to help completion by supporting --completion-helper option" (I think
> that is a sensible approach).  My comment was about Denton's patch,
> which reduced the hard-coded list of format-patch options (i.e. the
> first hunk) but had to add back many of them to send-email's
> completion (i.e. the last hunk)---overall, it did not help reducing
> the number of options hardcoded in the script.
> 
> If it makes sense to complete all options to format-patch to
> send-email, then as you outlined, grabbing them out of format-patch
> with the --completion-helper option at runtime, and using them to
> complete both format-patch and send-email would be a good idea.  And
> that should be doable even before send-email learns how to list its
> supported options to help the completion.

OK how about this?

Minimal changes in send-email.perl and no duplication in
_git_send_email(). I could do $(git format-patch
--git-completion-helper) directly from _git_send_email() too but we
lose caching.

-- 8< --
Subject: [PATCH] completion: use __gitcomp_builtin for format-patch

This helps format-patch gain completion for a couple new options,
notably --range-diff.

Since send-email completion relies on $__git_format_patch_options
which is now reduced, we need to do something not to regress
send-email completion.

The workaround here is implement --git-completion-helper in
send-email.perl just as a bridge to "format-patch --git-completion-helper".
This is enough to use __gitcomp_builtin on send-email (to take
advantage of caching).

In the end, send-email.perl can probably reuse the same info it passes
to GetOptions() to generate full --git-completion-helper output so
that we don't need to keep track of its options in git-completion.bash
anymore. But that's something for another boring day.

Helped-by: Denton Liu 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 16 ++--
 git-send-email.perl|  8 
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index db7fd87b6b..8409978793 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1532,13 +1532,9 @@ _git_fetch ()
__git_complete_remote_or_refspec
 }
 
-__git_format_patch_options="
-   --stdout --attach --no-attach --thread --thread= --no-thread
-   --numbered --start-number --numbered-files --keep-subject --signoff
-   --signature --no-signature --in-reply-to= --cc= --full-index --binary
-   --not --all --cover-letter --no-prefix --src-prefix= --dst-prefix=
-   --inline --suffix= --ignore-if-in-upstream --subject-prefix=
-   --output-directory --reroll-count --to= --quiet --notes
+__git_format_patch_extra_options="
+   --full-index --not --all --no-prefix --src-prefix=
+   --dst-prefix= --notes
 "
 
 _git_format_patch ()
@@ -1551,7 +1547,7 @@ _git_format_patch ()
return
;;
--*)
-   __gitcomp "$__git_format_patch_options"
+   __gitcomp_builtin format-patch 
"$__git_format_patch_extra_options"
return
;;
esac
@@ -2081,7 +2077,7 @@ _git_send_email ()
return
;;
--*)
-   __gitcomp "--annotate --bcc --cc --cc-cmd --chain-reply-to
+   __gitcomp_builtin send-email "--annotate --bcc --cc --cc-cmd 
--chain-reply-to
--compose --confirm= --dry-run --envelope-sender
--from --identity
--in-reply-to --no-chain-reply-to --no-signed-off-by-cc
@@ -2090,7 +2086,7 @@ _git_send_email ()
--smtp-server-port --smtp-encryption= --smtp-user
--subject --suppress-cc= --suppress-from --thread --to
--validate --no-validate
-   $__git_format_patch_options"
+   $__git_format

Re: [PATCH v1] add: speed up cmd_add() by utilizing read_cache_preload()

[Duy Nguyen]

On Sat, Nov 3, 2018 at 1:38 AM Junio C Hamano  wrote:
>
> Duy Nguyen  writes:
>
> > On Fri, Nov 2, 2018 at 2:32 PM Ben Peart  wrote:
> >>
> >> From: Ben Peart 
> >>
> >> During an "add", a call is made to run_diff_files() which calls
> >> check_remove() for each index-entry.  The preload_index() code distributes
> >> some of the costs across multiple threads.
> >
> > Instead of doing this site by site. How about we make read_cache()
> > always do multithread preload?
>
> I suspect that it would be a huge performance killer.
>
> Many codepaths do not even want to know if the working tree files
> have been modified, even though they need to know what's in the
> index.  Think "git commit-tree", "git diff --cached", etc.

Ah. I keep forgetting read_cache_preload is loading the index _and_
refreshing. I thought the two had some different semantics but failed
to see it last time.
-- 
Duy

Re: submodule support in git-bundle

[Duy Nguyen]

On Fri, Nov 2, 2018 at 6:09 PM Stefan Beller  wrote:
>
> On Fri, Nov 2, 2018 at 9:10 AM Duy Nguyen  wrote:
> >
> > I use git-bundle today and it occurs to me that if I want to use it to
> > transfer part of a history that involves submodule changes, things
> > aren't pretty. Has anybody given thought on how to do binary history
> > transfer that contains changes from submodules?
> >
> > Since .bundle files are basically .pack files, i'm not sure if it's
> > easy to bundle multiple pack files (one per repo)...
>
> That is a really good discussion starter!
>
> As bundles are modeled after the fetch protocol, I would
> redirect the discussion there.
>
> The new fetch protocol could support sending more than
> one pack, which could be for both the superproject as
> well as the relevant submodule updates (i.e. what is recorded
> in the superproject) based on a new capability.
>
> We at Google have given this idea some thought, but from a
> different angle: As you may know currently Android uses the
> repo tool, which we want to replace with Gits native submodules
> eventually. The repo tool tests for each repository to clone if
> there is a bundle file for that repository, such that instead of
> cloning the repo, the bundle can be downloaded and then
> a catch-up fetch can be performed. (This helps the Git servers
> as well as the client, the bundle can be hosted on a CDN,
> which is faster and cheaper than a git server for us).
>
> So we've given some thought on extending the packfiles in the
> fetch protocol to have some redirection to a CDN possible,
> i.e. instead of sending bytes as is, you get more or less a "todo"
> list, which might be
> (a) take the following bytes as is (current pack format)
> (b) download these other bytes from $THERE
> (possibly with a checksum)
> once the stream of bytes is assembled, it will look like a regular
> packfile with deltas etc.
>
> This offloading-to-CDN (or "mostly resumable clone" in the
> sense that the communication with the server is minimal, and
> you get most of your data via resumable http range-requests)
> sounds like complete offtopic, but is one of the requirements
> for the repo to submodule migration, hence I came to speak of it.

Hm.. so what you're saying is, we could have a pack file that lists
other (real) pack files and for the bundle case they are all in the
same file. And "download from $THERE" in this case is "download at
this file offset"? That might actually work.

> Did you have other things in mind, on a higher level?
> e.g. querying the bundle and creating submodule bundles
> based off the superproject bundle? 'git bundle create' could
> learn the --recurse-submodules option, which then produces
> multiple bundle files without changing the file formats.

This is probably the simplest way to support submodules. I just
haven't really thought much about it (the problem just came up to me
like 2 hours ago). Two problems with this are convenience (I don't
want to handle multiple files) and submodule info (which pack should
be unbundled on which submodule?). But I suppose if "git bundle"
produces a tarball of these bundle files then you solve both.

But of course there may be other and better options like what you
described above. If in long term we have "pack with hyperlinks" anyway
for resumable clone and other fancy stuff then reusing the same
mechanism for bundles makes sense, less maintenance burden.
-- 
Duy

submodule support in git-bundle

[Duy Nguyen]

I use git-bundle today and it occurs to me that if I want to use it to
transfer part of a history that involves submodule changes, things
aren't pretty. Has anybody given thought on how to do binary history
transfer that contains changes from submodules?

Since .bundle files are basically .pack files, i'm not sure if it's
easy to bundle multiple pack files (one per repo)...
-- 
Duy

Re: Understanding pack format

[Duy Nguyen]

On Fri, Nov 2, 2018 at 7:19 AM Junio C Hamano  wrote:
>
> Farhan Khan  writes:
>
> > ...Where is this in the git code? That might
> > serve as a good guide.
>
> There are two major codepaths.  One is used at runtime, giving us
> random access into the packfile with the help with .idx file.  The
> other is used when receiving a new packstream to create an .idx
> file.

The third path is copying/reusing objects in
builtin/pack-objects.c::write_reuse_object(). Since it's mostly
encoding the header of new objects in pack, it could also be a good
starting point. Then you can move to write_no_reuse_object() and get
how the data is encoded, deltified or not (yeah not parsed, but I
think it's more or less the same thing conceptually).
-- 
Duy