Re: git-gui: disable the "loose objects popup" dialog?
On 2019-10-01 2:00 p.m., Pratyush Yadav wrote: So here's what I propose: why don't we try to do something similar? What about running `git-gc --auto` in the background when the user makes a commit (which I assume is the most common operation in git-gui). This would be disabled when the user sets gc.auto to 0. This way, we keep a similar experience to the command line in case of auto-gc, and we get rid of the prompt. People who don't want auto-compression can just set gc.auto to 0, which they should do anyway. Thoughts? +1 from me. M.
Re: [PATCH] gitk: Add horizontal scrollbar to the files list
On 2019-10-01 6:08 a.m., Bert Wesarg wrote: Wrapping filenames is an unexpected experience in UX design. Disable wrapping and add a horizontal scrollbar to the files list to remove this. (Thanks for working on gitk and git-gui!) I have to say I'm mildly opposed to this change. The reason is that having to scroll to see the end of the filename is extra work, and it's work that would have to be repeated as one navigates between commits in the same area of code. Git-gui has scrollbars for its filename panes, and I find them more of a hassle that gitk's wrapping. (The horizontal scrollbar might work better if it defaulted to scrolling all the way to the *right* instead of to the left.) But I would instead prefer there to be some visual indication that the filename was wrapped. Maybe indent the wrapped lines? Or how about contracting the file path with an ellipsis (...), like "git diff --stat"? M. Signed-off-by: Bert Wesarg --- gitk | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/gitk b/gitk index abe4805..bf2a061 100755 --- a/gitk +++ b/gitk @@ -2477,13 +2477,16 @@ proc makewindow {} { -background $bgcolor -foreground $fgcolor \ -font mainfont \ -tabs [list $indent [expr {2 * $indent}]] \ - -yscrollcommand ".bright.sb set" \ + -xscrollcommand ".bright.sbx set" \ + -yscrollcommand ".bright.sby set" \ -cursor [. cget -cursor] \ - -spacing1 1 -spacing3 1 + -spacing1 1 -spacing3 1 -wrap none lappend bglist $cflist lappend fglist $cflist -${NS}::scrollbar .bright.sb -command "$cflist yview" -pack .bright.sb -side right -fill y +${NS}::scrollbar .bright.sbx -orient horizontal -command "$cflist xview" +${NS}::scrollbar .bright.sby -orient vertical -command "$cflist yview" +pack .bright.sbx -side bottom -fill x +pack .bright.sby -side right -fill y pack $cflist -side left -fill both -expand 1 $cflist tag configure highlight \ -background [$cflist cget -selectbackground]
Re: git-gui: disable the "loose objects popup" dialog?
On 2019-09-26 3:15 p.m., Pratyush Yadav wrote: On 26/09/19 08:54PM, Johannes Sixt wrote: Am 26.09.19 um 19:31 schrieb Birger Skogeng Pedersen: Every once in a while, I get the "This repository currently has approximately (some number) loose objects." popup dialog. I don't want to sound arrogant, but I find this popup along with the dialog showing after that prints the result of the compression, immensely annoying. And I've seen people mention before that they would, in some casese, rather not have to deal with the dialog[0]. [0] https://stackoverflow.com/questions/1106529/how-to-skip-loose-object-popup-when-running-git-gui I get that git-gui merely wants to resolve a performance issue. But personally I'd prefer if git could just assume I always wanted to compress the database, and automatically do it without bugging me with the popups. I dislike the popup, too. But I want total control over my repository: No automatic compression behind my back, in particular, when that expires reflogs, and git-gui does that. I agree. Doing stuff like this in background by default is not the best idea IMHO. If the user asks explicitly, fine, but don't do it by default. I propose we implement the following options in git-gui: - ignore loose objects (do not show the popup), disabled by default. Reading the Stackoverflow link, it seems this is already possible via an undocumented config variable "gui.gcwarning". I haven't tried using it though, but I see no reason for it to not work (looking at git-gui.sh:4141). I use this, and it works. I haven't seen that dialog in years of near-daily repo usage. Maybe we should add this variable in the options dialog, so people at least know it exists? My experience with qui.gcwarning (i.e. that git-gui hasn't compressed my repo in a very long time) suggests that we can just get rid of this part of git-gui. I seem to recall that this was suggested the last time this was discussed, because the rest of git's auto-gc machinery is now working quite well (compared to when git-gui was first introduced). M. - automatically, silently compress the database, without prompt. Also disabled by default. What about a configurable limit, but still show the dialog? Do people really care that much about configuring this limit to warrant something like this? Talking about auto compression, would it be a better idea to let users disable the dialog, and then if they do want auto compression, they can just run a cron job (or the Windows equivalent) to do this on their repos? What reasons do people have to have this feature in git-gui, instead of running cron jobs?
Re: Git Gui - enhancement suggestion - Can a double click on the file name in the “unstaged” area move the item to “staged changes”
On 2019-09-13 10:32 a.m., Pratyush Yadav wrote: On 13/09/19 12:24PM, Allan Ford wrote: Dear Git Authors, Not a bug, but a suggestion consideration for “Git Gui” Can a double click on the file name in the “unstaged” area move the item to “staged changes” .. (rather than having to click on the small icon to the left of the file name?) It has been something on my radar for some time. Shouldn't be something too difficult to do. While I like the idea in general, I have a question that I'd like to ask other git-gui users: If we implement something like this, what happens when you single-click on the icon? Do we treat that as a stage/unstage command? If we keep the legacy behaviour of single-click on the icon stages/unstages, then a part of the row is single-click and the rest double-click. If we make an entire row of the stage/unstage widget double click, it messes with people who are already used to it. Is partial single and partial double click behaviour acceptable? Or should we make the entire row double click only? Or something else that I missed? I've always felt this was a bit of user-experience failure on git-gui's part. Single-click should not behave differently just because you click the icon. I've seen many new git-gui users find this (mildly) confusing. I'd be happy if the click behavior was consistent across the entire row: single-click to select, double-click to stage/unstage, and there's nothing special about clicking the icon. I personally don't think it would be hard to adjust to that. I guarantee you that if double-click support is added while preserving the icon-single-click, users will get tripped up when they double-click the icon and accidentally stage two files. M.
Re: [PATCH 2/2] git-gui: add hotkey to toggle "Amend Last Commit" check button/menu
On 2019-09-13 3:50 a.m., Birger Skogeng Pedersen wrote: Hi Marc and Philip, On 12/09/2019 22:34, Marc Branchaud wrote: I disagree! Who expects anything to work properly when capslock is on? Me :-) Fair enough, though I imagine you have a pretty narrow definition of "anything". :) On Fri, Sep 13, 2019 at 12:23 AM Philip Oakley wrote: I'd tend to agree. In other areas the use of shift is often used as the complement of the unshifted action, so it does feel 'odd'. Thus it could be used directly as the bool for amend or direct commit. This all assumes that Caps Lock is equivalent to having the shift on, rather than being a special extra key. It seems all the Ctrl+(lowercase character) hotkeys in git-gui have an equivalent Ctrl+(uppercase character). So for this feature, we should keep the Ctrl+E bind aswell as the Ctrl+e bind. If nothing else, to keep it consistent with the rest of the hotkey bindings. Ah, OK. I agree that keeping git-gui internally consistent trumps the other considerations. M. But honestly, (as Marc pointed out) it is a quite weird that Ctrl+Shift+(character) has the excact same function as Ctrl+(character). Perhaps we should find another way to bind the hotkeys, where the state of Caps Lock doesn't matter? If possible. Birger
Re: [PATCH 2/2] git-gui: add hotkey to toggle "Amend Last Commit" check button/menu
On 2019-09-12 12:29 p.m., Pratyush Yadav wrote: On 12/09/19 08:05AM, Birger Skogeng Pedersen wrote: Hi Pratyush, On Wed, Sep 11, 2019 at 10:55 PM Pratyush Yadav wrote: Also, I notice that the bindings for other letters have the same function bound for both small and capital letters (IOW, same behavior with shift held and released). I don't necessarily think that is a great idea. It is a pretty common pattern to have, say Ctrl+a, do something, and Ctrl+Shift+a, do something else. Just want to pick your brain on whether you think we should do the same thing for both Ctrl+e and for Ctrl+E (aka Ctrl+Shift+e), or just bind it to Ctrl+e, and leave Ctrl+E for something else. I just tested what happens when you press Ctrl+e while Caps Lock is enabled; the Ctrl+e binding is not invoked. That's probably why other key bindings have the same function bound for both lower- and upper-case letters, to have the same behaviour with/without Caps Lock enabled. With that in mind, we should probably bind Ctrl+E aswell. Nice catch! Makes sense to have the same behaviour for both caps lock enabled and disabled. (I've been a git-gui user for many years...) I disagree! Who expects anything to work properly when capslock is on? M. Should I create and send a new patch? Yes, please do.
Re: [PATCH] new-workdir: Never try to recurse into submodules on the initial checkout.
On 2019-01-14 4:58 p.m., Jonathan Nieder wrote: Hi, Junio C Hamano wrote: Jonathan Nieder writes: Marc Branchaud wrote: The new workdir is empty before the checkout, so attempts to recurse into a non-existent submodule directory fail. Signed-off-by: Marc Branchaud --- Thanks for reporting. Can you describe the error message when it fails here? [...] IOW, at the point in that script where we call "git checkout -f", if we changed it to "git checkout --recurse-submodules -f", what breaks and why? Shouldn't it succeed instead? I think that's a similar question to the one I asked. But I have a good guess about what goes wrong. It's related to the same issue as the "git worktree" problem Marc described. Inside the superproject's $GIT_DIR, we see config modules/ a/ config b/ config ... The question is what to do with the modules/ directory when you have multiple working directories making use of the refs and objects from this $GIT_DIR. In general, the most useful answer is that the additional working directories should make use for modules/ from this $GIT_DIR as well. After all, each submodule has its own refs and objects, and the same motivation that pushes us to share the refs and objects from the superproject would drive us to share them in the submodules as well. However, if you do this in the most naive way, it will not work. In the config file, there is a core.worktree setting that ensures that commands run from a submodule affect the correct working directory. Which worktree should it point to? All of them. There's still an obvious "most useful" answer: each submodule should contain its own worktrees/ directory with metadata specific to each worktree. This should work fine and is the future work that Marc and I alluded to. Let me call it (*), for later reference. Anything done today is papering over the sad truth that that future work (*) has not been done yet. contrib/workdir is currently naive about all this: it does *not* symlink across the modules/ directory, so each workdir gets its own independent copy of all the submodules. Which kind of defeats the point of this kind of setup. That said, it's better than nothing at all, which is why Marc proposes making it not attempt to check out the submodules right away, instead permitting the user to make the best of things. I suppose another thing that is missing is a warning message to ensure the user knows that once (*) arrives, they need to be ready for it. (Or not: this is contrib/workdir, and there would be no need to make use of it in place of "git worktree" once that moment arrives.) To reiterate, this is all about papering over (*) not having been done. Marc, did I understand correctly? Yup! I just hope to keep git-new-workdir hobbling along until worktree can fully replace it. I agree with Junio about what should happen when submodule.recurse=true. But that work should be done in worktree instead of git-new-workdir. M. Thanks, Jonathan
Re: [PATCH] new-workdir: Never try to recurse into submodules on the initial checkout.
On 2019-01-14 4:34 p.m., Jonathan Nieder wrote: Hi, Marc Branchaud wrote: The new workdir is empty before the checkout, so attempts to recurse into a non-existent submodule directory fail. Signed-off-by: Marc Branchaud --- Thanks for reporting. Can you describe the error message when it fails here? The error is: fatal: exec '--super-prefix=external/submodule/': cd to 'external/submodule' failed: No such file or directory The created workdir has only the .git directory. The .git/HEAD file contains the expected ref, so the workdir repo's status simply shows that everything has been deleted. Note that git-worktree also fails when submodule.recurse=true, with the same error: # git worktree add ~/Code/foo/test-worktree Preparing worktree (new branch 'test-worktree') fatal: exec '--super-prefix=external/submodule/': cd to 'external/submodule' failed: No such file or directory error: Submodule 'external/submodule' could not be updated. error: Submodule 'external/submodule' cannot checkout new HEAD. fatal: Could not reset index file to revision 'HEAD'. I had assumed that this was simply an aspect of submodules not working, so I was holding off reporting it until more of the submodule support was complete. Until the worktree command supports submodules I've gone back to using the git-new-workdir script, but it fails if my config has submdodule.recurse=true. Oh, dear. In general, the project does a better job at supporting "git worktree" than "git new-workdir", but I don't blame you about this. Noting locally as another vote for getting submodules to play well with worktrees soon. [...] contrib/workdir/git-new-workdir | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/workdir/git-new-workdir b/contrib/workdir/git-new-workdir index 888c34a521..5de1dc3c58 100755 --- a/contrib/workdir/git-new-workdir +++ b/contrib/workdir/git-new-workdir @@ -102,4 +102,4 @@ trap - $siglist # checkout the branch (either the same as HEAD from the original repository, # or the one that was asked for) -git checkout -f $branch +git -c submodule.recurse=false checkout -f $branch nit: can this use "git checkout --no-recurse-submodules" instead of -c? In general, we tend to recommend that kind of option instead of --config in scripts. --no-recurse-submodules does work. I'll send a v2. M. Thanks, Jonathan
[PATCHv2] new-workdir: Never try to recurse into submodules on the initial checkout.
The new workdir is empty before the checkout, so attempts to recurse into a non-existent submodule directory fail. Signed-off-by: Marc Branchaud --- Changed to use --no-recurse-submodules instead of -c submodule.recurse=false, as Jonathan suggested. M. contrib/workdir/git-new-workdir | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/workdir/git-new-workdir b/contrib/workdir/git-new-workdir index 888c34a521..d88765e73f 100755 --- a/contrib/workdir/git-new-workdir +++ b/contrib/workdir/git-new-workdir @@ -102,4 +102,4 @@ trap - $siglist # checkout the branch (either the same as HEAD from the original repository, # or the one that was asked for) -git checkout -f $branch +git checkout --no-recurse-submodules -f $branch -- 2.20.1.1.gfb6d716d28
[PATCH] new-workdir: Never try to recurse into submodules on the initial checkout.
The new workdir is empty before the checkout, so attempts to recurse into a non-existent submodule directory fail. Signed-off-by: Marc Branchaud --- Until the worktree command supports submodules I've gone back to using the git-new-workdir script, but it fails if my config has submdodule.recurse=true. With this patch, the checkout succeeds and the workdir has empty submodules, which is the script's normal behaviour. M. contrib/workdir/git-new-workdir | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/workdir/git-new-workdir b/contrib/workdir/git-new-workdir index 888c34a521..5de1dc3c58 100755 --- a/contrib/workdir/git-new-workdir +++ b/contrib/workdir/git-new-workdir @@ -102,4 +102,4 @@ trap - $siglist # checkout the branch (either the same as HEAD from the original repository, # or the one that was asked for) -git checkout -f $branch +git -c submodule.recurse=false checkout -f $branch -- 2.20.1.1.gfb6d716d28
Wherefor worktrees?
On 2018-09-26 11:48 AM, Duy Nguyen wrote: I believe the main selling point of multiple worktrees is sharing refs. You could easily avoid expensive clones with --local, but synchronizing between different clones is not very convenient. Other than that, different worktrees tend to behave like separate clones. Sharing hooks is also useful, but yes mainly the refs. I love being able to work in more than one branch at a time. I often have a couple of ongoing big, messy topics, and being able to easily jump onto some release branch for a quick bugfix, without having to first stash things or finish an interactive rebase or fix a conflicting merge, is a godsend. And the reason I use worktrees for this, instead of clones, is for the shared refs. It makes sense to me that I'm working with different checkouts from a single repo, with all my local branches and local tags. "git fetch" updates the remote refs regardless of which worktree I'm in when I run it. The setup is lightweight and efficient; it's just how I want to work. Having used git-new-workdir for a long time, it's main deficiency for me is submodules (the shared bisection state didn't bother me much). It would be nice if all my worktrees' submodules also shared refs. That's "nice", but not "essential". Mainly it would be convenient if a recursive-submodule fetch performed in one worktree updated the submodule refs in my other worktrees. Similarly, if I create a local branch in a submodule in one worktree, it would be nice to see that branch in the submodule in other worktrees. Again, "nice", but probably just because I've lived with git-new-workdir's limitations for so long that I'm used to them. That said, I really appreciate Duy's work here -- thanks! Git deserves to have a cool feature like worktrees be part of its standard toolkit. M. This leaves a gray area where other things should be shared or not. I think the preference (or default mode) is still _not_ shared (*). Sharing more things (besides refs and object database) is just a new opportunity popping up when we implement multiple worktrees. Since multiple worktrees (or clones before its time) are grouped together, sometimes you would like to share common configuration. We could sort of achieve this already with includeIf but again not as convenient. (*) real life is not that simple. Since refs are shared, including _remotes_ refs, so configuration related to remotes should also be shared, or it will be a maintenance nightmare. Which is why $GIT_DIR/config so far has been shared besides the two exceptions that are core.bare and core.worktree. And I really like to get rid of these exceptions. Is there a better way to achieve that without the downside of multiple worktrees (e.g. configuration need to be uniform)? Is there a better way to achieve sharing refs between clones? I gave it a minute but couldn't come up with a good answer :( (*) "git config --worktree" points back to "config" file when this extension is not present so that it works in any setup. Shouldn't it barf and error out instead? The intention is a uniform interface/api that works with both single and multiple worktrees configurations. Otherwise in your scripts you would need to write "if single worktree, do this, else do that". If "git config --worktree" works with both, the existing scripts can be audited and updated just a bit, adding "--worktree" where the config should not be shared, and we're done. A user who hasn't enabled the extension uses --worktree option and misled to believe that the setting affects only a single worktree, even though the change is made globally---that does not sound like a great end-user experience. I was talking about a single worktree. But I think here you meant the user has multiple worktrees, but the extension is not enabled. I'm probably not clear in the commit message, but "git config" can detect that the extension has not been enabled, automatically enable it (and move core.bare and core.worktree (if present) to the main worktree's private config), so "git config --worktree" will never share the change. But perhaps the user should be made aware of this situation and asked to explicitly enable the extension instead? It's certainly a more conservative approach.
[PATCH] fetch: Ensure that fetch.recurseSubmodules overrides submodule.recurse.
Also document this fact. Signed-off-by: Marc Branchaud --- I ran into this bug when I had both fetch.recurseSubmodules=on-demand and submodule.recurse=true, and submodule.recurse was set *after* fetch.recurseSubmodules in my config. The fix ensures that fetch.recurseSubmodules always overrides submodule.recurse. If neither is set then fetch still behaves as if fetch.recurseSubmodules=on-demand (the documented default). I'm not sure if this is the most elegant implementation, but it gets the job done. M. Documentation/config.txt | 6 -- builtin/fetch.c | 5 - 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index eb66a11975..67b0adc1d4 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1514,7 +1514,8 @@ fetch.recurseSubmodules:: recurse at all when set to false. When set to 'on-demand' (the default value), fetch and pull will only recurse into a populated submodule when its superproject retrieves a commit that updates the submodule's - reference. + reference. This option overrides the more general submodule.recurse + option, for the `fetch` command. fetch.fsckObjects:: If it is set to true, git-fetch-pack will check all fetched @@ -3465,7 +3466,8 @@ submodule.active:: submodule.recurse:: Specifies if commands recurse into submodules by default. This applies to all commands that have a `--recurse-submodules` option, - except `clone`. + except `clone`. Also, the `fetch` command's behaviour can be specified + independently with the fetch.recurseSubmodules option. Defaults to false. submodule.fetchJobs:: diff --git a/builtin/fetch.c b/builtin/fetch.c index 61bec5d213..08b8bf2741 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -60,6 +60,7 @@ static struct transport *gsecondary; static const char *submodule_prefix = ""; static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT; static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND; +static int recurse_submodules_set_explicitly = 0; static int shown_url = 0; static struct refspec refmap = REFSPEC_INIT_FETCH; static struct list_objects_filter_options filter_options; @@ -78,7 +79,8 @@ static int git_fetch_config(const char *k, const char *v, void *cb) return 0; } - if (!strcmp(k, "submodule.recurse")) { + if (!strcmp(k, "submodule.recurse") && + !recurse_submodules_set_explicitly) { int r = git_config_bool(k, v) ? RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF; recurse_submodules = r; @@ -88,6 +90,7 @@ static int git_fetch_config(const char *k, const char *v, void *cb) max_children = parse_submodule_fetchjobs(k, v); return 0; } else if (!strcmp(k, "fetch.recursesubmodules")) { + recurse_submodules_set_explicitly = 1; recurse_submodules = parse_fetch_recurse_submodules_arg(k, v); return 0; } -- 2.19.0.1.g5109f9487a
Re: Is rebase --force-rebase any different from rebase --no-ff?
On 2018-05-09 03:46 PM, Ilya Kantor wrote: I tried to compare --force-rebase VS --no-ff for the following repository: http://jmp.sh/E7TRjcL There's no difference in the resulf of: git rebase --force-rebase 54a4 git rebase --no-ff 54a4 (rebases all 3 commits of feature) Also, there's no difference in interactive mode: git rebase --force-rebase -i 54a4 git rebase --no-ff -i 54a4 (picks all 3 commits of feature) Is there a case where --no-ff differs from --force-rebase? So now that "rebase -i" respects --force-rebase, the question is what to do about it: 1. Teach "rebase -i" to stop respecting --force-rebase (restoring the original intent when --no-ff was introduced)? 2. Deprecate --no-ff? 3. Deprecate --force-rebase? As a heavy rebase user, I find --no-ff more intuitive than --force-rebase. I'd be in favour of option 3, and keeping just --no-ff (with -f as a synonym). M. --- Best Regards, Ilya Kantor On Wed, May 9, 2018 at 10:27 PM, Marc Branchaud wrote: On 2018-05-09 02:21 PM, Stefan Beller wrote: +cc Marc and Johannes who know more about rebase. On Wed, May 9, 2018 at 9:01 AM, Ilya Kantor wrote: Right now in "git help rebase" for --no-ff: "Without --interactive, this is a synonym for --force-rebase." But *with* --interactive, is there any difference? I found https://code.googlesource.com/git/+/b499549401cb2b1f6c30d09681380fd519938eb0 from 2010-03-24 In the original discussion around this option [1], at one point I proposed teaching rebase--interactive to respect --force-rebase instead of adding a new option [2]. Ultimately --no-ff was chosen as the better user interface design [3], because an interactive rebase can't be "forced" to run. At the time, I think rebase--interactive only recognized --no-ff. That might have been muddled a bit in the migration to rebase--helper.c. Looking at it now, I don't have a strong opinion about keeping both options or deprecating one of them. M. [1] https://public-inbox.org/git/4b9fd9c1.9060...@xiplink.com/t/ [2] https://public-inbox.org/git/1269361187-31291-1-git-send-email-marcn...@xiplink.com/ [3] https://public-inbox.org/git/7vzl1yd5j4@alter.siamese.dyndns.org/ Teach rebase the --no-ff option. For git-rebase.sh, --no-ff is a synonym for --force-rebase. For git-rebase--interactive.sh, --no-ff cherry-picks all the commits in the rebased branch, instead of fast-forwarding over any unchanged commits. --no-ff offers an alternative way to deal with reverted merges. Instead of "reverting the revert" you can use "rebase --no-ff" to recreate the branch with entirely new commits (they're new because at the very least the committer time is different). This obviates the need to revert the reversion, as you can re-merge the new topic branch directly. Added an addendum to revert-a-faulty-merge.txt describing the situation and how to use --no-ff to handle it. which sounds as if there is? Stefan
Re: Is rebase --force-rebase any different from rebase --no-ff?
On 2018-05-09 02:21 PM, Stefan Beller wrote: +cc Marc and Johannes who know more about rebase. On Wed, May 9, 2018 at 9:01 AM, Ilya Kantor wrote: Right now in "git help rebase" for --no-ff: "Without --interactive, this is a synonym for --force-rebase." But *with* --interactive, is there any difference? I found https://code.googlesource.com/git/+/b499549401cb2b1f6c30d09681380fd519938eb0 from 2010-03-24 In the original discussion around this option [1], at one point I proposed teaching rebase--interactive to respect --force-rebase instead of adding a new option [2]. Ultimately --no-ff was chosen as the better user interface design [3], because an interactive rebase can't be "forced" to run. At the time, I think rebase--interactive only recognized --no-ff. That might have been muddled a bit in the migration to rebase--helper.c. Looking at it now, I don't have a strong opinion about keeping both options or deprecating one of them. M. [1] https://public-inbox.org/git/4b9fd9c1.9060...@xiplink.com/t/ [2] https://public-inbox.org/git/1269361187-31291-1-git-send-email-marcn...@xiplink.com/ [3] https://public-inbox.org/git/7vzl1yd5j4@alter.siamese.dyndns.org/ Teach rebase the --no-ff option. For git-rebase.sh, --no-ff is a synonym for --force-rebase. For git-rebase--interactive.sh, --no-ff cherry-picks all the commits in the rebased branch, instead of fast-forwarding over any unchanged commits. --no-ff offers an alternative way to deal with reverted merges. Instead of "reverting the revert" you can use "rebase --no-ff" to recreate the branch with entirely new commits (they're new because at the very least the committer time is different). This obviates the need to revert the reversion, as you can re-merge the new topic branch directly. Added an addendum to revert-a-faulty-merge.txt describing the situation and how to use --no-ff to handle it. which sounds as if there is? Stefan
Re: [RFC PATCH] checkout: Force matching mtime between files
On 2018-04-27 01:03 PM, Duy Nguyen wrote: On Wed, Apr 25, 2018 at 5:18 PM, Marc Branchaud wrote: The best approach to do so is to have those people do the "touch" thing in their own post-checkout hook. People who use Git as the source control system won't have to pay runtime cost of doing the touch thing, and we do not have to maintain such a hook script. Only those who use the "feature" would. The post-checkout hook approach is not exactly straightforward. I am revisiting this because I'm not even happy with my post-checkout-modified hook suggestion, so.. Naively, it's simply for F in `git diff --name-only $1 $2`; do touch "$F"; done But consider: * Symlinks can cause the wrong file to be touched. (Granted, Michał's proposed patch also doesn't deal with symlinks.) Let's assume that a hook can be crafted will all possible sophistication. There are still some fundamental problems: OK so this one could be tricky to get right, but it's possible. * In a "file checkout" ("git checkout -- path/to/file"), $1 and $2 are identical so the above loop does nothing. Offhand I'm not even sure how a hook might get the right files in this case. This is a limitation of the current post-checkout hook. $3==0 from the hook lets us know this is not a branch switch, but it does not really tell you the affected paths. If it somehow passes all the given pathspec to you, then you should be able to do "git ls-files -- $pathspec" which gives you the exact same set of paths that git-checkout updates. We could do this by setting $4 to "--" and put all the pathspecs in $5+ [1] e.g. "HEAD@{1} HEAD 0 -- path/to/file" in the above example. There is third case here, if you do "git checkout -- path/to/file" then it cannot be covered by the current design. I guess we could set $3 to '2' (retrieve from a tree) to indicate this in addition to 0 (from index) and 1 (from switching branch) and then $1 could be the tree in question (pathspecs are passed the same way above) [1] I wonder if we could have a more generic approach to pass pathspecs via environment, which could work for more than just this one hook. Not sure if it's a good idea though. I think there needs to be something other than listing all the paths in the command is viable, because it's too easy to hit some command-line-length limit. It would also be good if hook authors didn't have to re-invent the wheel of determining the changed paths for every corner-case. My first instinct is to write them one-per-line on the hook's stdin. That's probably not generic enough for most hooks, but it seems like a good approach for this proposal. Throwing them into a temporary file with a known name is also good --- better, I think, than stuffing them into an environment variable. M. * The hook has to be set up in every repo and submodule (at least until something like Ævar's experiments come to fruition). Either --template or core.hooksPath would solve this, or I'll try to get my "hooks.* config" patch in. I think that one is a good thing to do anyway because it allows more flexible hook management (and it could allow multiple hooks of the same name too). With this, we could avoid adding more command-specific config like core.fsmonitor or uploadpack.packObjectsHook which to me are hooks. Another option is extend core.hooksPath for searching multiple places instead of just one like AEvar suggested. * A fresh clone can't run the hook. This is especially important when dealing with submodules. (In one case where we were bit by this, make though that half of a fresh submodule clone's files were stale, and decided to re-autoconf the entire thing.) Both --template and config-based hooks should let you handle this case. So, I think if we improve the hook system to give more information (which is something we definitely should do), we could do this without adding special cases in git. I'm not saying that we should never add special cases, but at least this lets us play with different kinds of post-checkout activities before we decide which one would be best implemented in git.
Re: [RFC PATCH] checkout: Force matching mtime between files
On 2018-04-25 09:25 PM, Junio C Hamano wrote: Marc Branchaud writes: But Git is not an archiver (tar), but is a source code control system, so I do not think we should spend any extra cycles to "improve" its behaviour wrt the relative ordering, at least for the default case. Only those who rely on having build artifact *and* source should pay the runtime (and preferrably also the maintainance) cost. Anyone who uses "make" or some other mtime-based tool is affected by this. I agree that it's not "Everyone" but it sure is a lot of people. That's an exaggerated misrepresentation. Only those who put build artifacts as well as source to SCM *AND* depend on mtime are affected. A shipped tarball often contain configure.in as well as generated configure, so that consumers can just say ./configure without having the whole autoconf toolchain to regenerate it (I also heard horror stories that this is done to control the exact version of autoconf to avoid compatibility issues), but do people arrange configure to be regenerated from configure.in in their Makefile of such a project automatically when building the default target? Yes. I've seen many automake-generated Makefiles with "recheck" machinery that'll conveniently rerun autoconf if "necessary". (And those horror stories are true, BTW.) In any case, that is a tarball usecase, not a SCM one. No, it's an SCM case when you need to have the project's code as part of your own. I can't make everyone do things the Right Way, so I'm stuck using projects that are not 100% pure-source, because they "helpfully" release their code after the autoconf step for some reason. Are we all that sure that the performance hit is that drastic? After all, we've just done write_entry(). Calling utime() at that point should just hit the filesystem cache. I do not know about others, but I personally am more disburbed by the conceptual ugliness that comes from having to have such a piece of code in the codebase. The ugliness arises from the problem being solved. It's not git's fault that the world is so stupid. That git might be willing to suffer a bit of self-mutilation for the benefit of its users should be seen as a point of pride. M.
Re: [RFC PATCH] checkout: Force matching mtime between files
On 2018-04-25 04:48 AM, Junio C Hamano wrote: "Robin H. Johnson" writes: In the thread from 6 years ago, you asked about tar's behavior for mtimes. 'tar xf' restores mtimes from the tar archive, so relative ordering after restore would be the same, and would only rebuild if the original source happened to be dirty. This behavior is already non-deterministic in Git, and would be improved by the patch. But Git is not an archiver (tar), but is a source code control system, so I do not think we should spend any extra cycles to "improve" its behaviour wrt the relative ordering, at least for the default case. Only those who rely on having build artifact *and* source should pay the runtime (and preferrably also the maintainance) cost. Anyone who uses "make" or some other mtime-based tool is affected by this. I agree that it's not "Everyone" but it sure is a lot of people. Are we all that sure that the performance hit is that drastic? After all, we've just done write_entry(). Calling utime() at that point should just hit the filesystem cache. The best approach to do so is to have those people do the "touch" thing in their own post-checkout hook. People who use Git as the source control system won't have to pay runtime cost of doing the touch thing, and we do not have to maintain such a hook script. Only those who use the "feature" would. The post-checkout hook approach is not exactly straightforward. Naively, it's simply for F in `git diff --name-only $1 $2`; do touch "$F"; done But consider: * Symlinks can cause the wrong file to be touched. (Granted, Michał's proposed patch also doesn't deal with symlinks.) Let's assume that a hook can be crafted will all possible sophistication. There are still some fundamental problems: * In a "file checkout" ("git checkout -- path/to/file"), $1 and $2 are identical so the above loop does nothing. Offhand I'm not even sure how a hook might get the right files in this case. * The hook has to be set up in every repo and submodule (at least until something like Ævar's experiments come to fruition). * A fresh clone can't run the hook. This is especially important when dealing with submodules. (In one case where we were bit by this, make though that half of a fresh submodule clone's files were stale, and decided to re-autoconf the entire thing.) I just don't think the hook approach can completely solve the problem. I appreciate Ævar's concern that there are more than just two mtime requests floating around. But I think git's users are best served by a built-in approach, with a config setting to control the desired mtime handling (defaulting to the current behaviour). People who want a different mtime solution will at least have a clear place in the code to propose a patch. M.
Re: [RFC PATCH] checkout: Force matching mtime between files
On 2018-04-13 01:01 PM, Michał Górny wrote: Currently git does not control mtimes of files being checked out. This means that the only assumption you could make is that all files created or modified within a single checkout action will have mtime between start time and end time of this checkout. The relations between mtimes of different files depend on the order in which they are checked out, filesystem speed and timestamp precision. Thanks for scratching this old itch [1]! Big +1 from me. We've had incremental smoke-test rebuilds fail because of inconsistent file timestamps. M. [1] https://public-inbox.org/git/50c79d1f.1080...@xiplink.com/ Git repositories sometimes contain both generated and respective source files. For example, the Gentoo 'user syncing' repository combines source ebuild files with generated metadata cache for user convenience. Ideally, the 'git checkout' would be fast enough that (combined with low timestamp precision) all files created or modified within a single checkout would have matching timestamp. However, in reality the cache files may end up being wrongly 'older' than source file, requiring unnecessary recheck. The opposite problem (cache files not being regenerated when they should have been) might also occur. However, it could not be solved without preserving timestamps, therefore it is out of scope of this change. In order to avoid unnecessary cache mismatches, force a matching mtime between all files created by a single checkout action. This seems to be the best course of action. Matching mtimes do not trigger cache updates. They also match the concept of 'checkout' being an atomic action. Finally, this change does not break backwards compatibility as the new result is a subset of the possible previous results. For example, let's say that 'git checkout' creates two files in order, with respective timestamps T1 and T2. Before this patch, T1 <= T2. After this patch, T1 == T2 which also satisfies T1 <= T2. A similar problem was previously being addressed via git-restore-mtime tool. However, that solution is unnecessarily complex for Gentoo's use case and does not seem to be suitable for 'seamless' integration. The patch integrates mtime forcing via adding an additional member of 'struct checkout'. This seemed the simplest way of adding it without having to modify prototypes and adjust multiple call sites. The member is set to the current time in check_updates() function and is afterwards enforced by checkout_entry(). The patch uses utime() rather than futimes() as that seems to be the function used everywhere else in git and provided some MinGW compatibility code. Signed-off-by: Michał Górny --- cache.h| 1 + entry.c| 12 +++- unpack-trees.c | 1 + 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/cache.h b/cache.h index bbaf5c349..9f0a7c867 100644 --- a/cache.h +++ b/cache.h @@ -1526,6 +1526,7 @@ struct checkout { const char *base_dir; int base_dir_len; struct delayed_checkout *delayed_checkout; + time_t checkout_mtime; unsigned force:1, quiet:1, not_new:1, diff --git a/entry.c b/entry.c index 2101201a1..7ee5a6d28 100644 --- a/entry.c +++ b/entry.c @@ -411,6 +411,7 @@ int checkout_entry(struct cache_entry *ce, { static struct strbuf path = STRBUF_INIT; struct stat st; + int ret; if (topath) return write_entry(ce, topath, state, 1); @@ -473,5 +474,14 @@ int checkout_entry(struct cache_entry *ce, return 0; create_directories(path.buf, path.len, state); - return write_entry(ce, path.buf, state, 0); + ret = write_entry(ce, path.buf, state, 0); + + if (ret == 0 && state->checkout_mtime != 0) { + struct utimbuf t; + t.modtime = state->checkout_mtime; + if (utime(path.buf, &t) < 0) + warning_errno("failed utime() on %s", path.buf); + } + + return ret; } diff --git a/unpack-trees.c b/unpack-trees.c index e73745051..e1efefb68 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -346,6 +346,7 @@ static int check_updates(struct unpack_trees_options *o) state.quiet = 1; state.refresh_cache = 1; state.istate = index; + state.checkout_mtime = time(NULL); progress = get_progress(o);
Re: [PATCH V2] config: add --expiry-date
On 2017-11-14 01:21 AM, Christian Couder wrote: On Tue, Nov 14, 2017 at 3:04 AM, wrote: From: Haaris Description: This patch adds a new option to the config command. Uses flag --expiry-date as a data-type to covert date-strings to timestamps when reading from config files (GET). This flag is ignored on write (SET) because the date-string is stored in config without performing any normalization. Creates a few test cases and documentation since its a new feature. Motivation: A parse_expiry_date() function already existed for api calls, this patch simply allows the function to be used from the command line. Signed-off-by: Haaris Documentation/SubmittingPatches contains the following: "Also notice that a real name is used in the Signed-off-by: line. Please don't hide your real name." And there is the following example before that: Signed-off-by: Random J Developer So it looks like "a real name" actually means "a real firstname and a real surname". It would be nice if your "Signed-off-by:" could match this format. It might already match that format if Haaris lives in a society that only uses single names. Still, such names are unusual enough that it's good to check that new contributors are following the guidelines properly. M. Also if you have a "From:" line at the beginning of the patch, please make sure that the name there is tha same as on the "Signed-off-by:". Thanks for working on this, Christian.
Re: Recovering from gc errors
(It turned out that this problem is related to worktrees. CCing some worktree folks.) On 2017-11-14 12:53 AM, Jeff King wrote: On Mon, Nov 13, 2017 at 04:13:19PM -0500, Marc Branchaud wrote: Various incantations of "git show ... 9c355a7726e31" only fail with the same error, so I can't determine much about the problematic commit. Luckily I'm not particularly concerned with losing objects, as I push any important progress to named refs in backup repos. Doing "git show" will require looking at the parent commit to produce the diff. Probably "git show -s" would work. But in general for poking at corruption, something bare-bones like "git cat-file commit 9c355a77" is going to be your best bet. Thanks, I'd forgotten about cat-file (show's -s did not work). Only one or two of the bad commits could possibly belong in a submodule, so I don't think I'm seeing a worktree+submodule problem. There are some definite "rebase -i" commits (e.g. "fixup!"), and a lot of what were probably cherry-picks. I know I did these operations in a worktree (see below). But I would like to clean this up in my local repo so that gc stops failing. I tried simply removing this and other loose commits that trip up gc (i.e. the objects/9c/355a7726e31b3033b8e714cf7edb4f0a41d8d4 file -- there are 49 such files, all of which are several months old), but now gc complains of a bad tree object: You can't generally fix corruption issues by deleting objects[1]. The "source" that makes Git want to have these objects is the refs and reflogs. So your best bet is to find which of those point to the problematic objects and delete them. I'd start by seeing if the breakage is reachable from any refs: git rev-list --objects --all >/dev/null That command does succeed. If that command succeeds, then all your refs are intact and the problem is in the reflogs. You can try to figure out which, but I'd probably just blow them all away: rm -rf .git/logs Unfortunately, removing the logs directory does not fix "git gc". So I restored it. However I did find all of the bad SHAs in the HEAD logs of four of my worktrees. All of those worktrees have directories in .git/worktrees/, but "git worktree list" does not show two of them. "git worktree prune -v" displays and does nothing. (I do not want to play with --expire, because I'd rather keep my other worktrees.) I removed all of those worktrees' directories from .git/worktrees/, and now "git gc" succeeds. I've also removed those worktrees' working directories, as I don't really need them anymore. Thanks for your help! I'm willing to chalk this up to bugs in the early worktree code, unless one of the CC'd worktree developers thinks otherwise. An explicit "git worktree delete" command would be nice for manually cleaning things up. It's easy to just delete the directory, but having a "delete" command gives the user assurance that they're not missing something. M. If the rev-list fails, then one or more branch is corrupted. Unfortunately the usual efficient tools for asking "which branch contains this object" are likely to be broken by the corruption. But you can brute-force it, like: git for-each-ref --format='%(refname)' | while read ref; do git rev-list --objects "$ref" >/dev/null 2>&1 || echo "$ref is broken" done Hopefully that turns up only branches with little value, and you can delete them: git update-ref -d $broken_ref -Peff [1] A note on my "you can't fix corruption by deleting objects". Since abcb86553d (pack-objects: match prune logic for discarding objects, 2014-10-15) , git-gc also traverses the history graph of unreachable but "recent" objects. This is to keep whole chunks of the history graph intact during the gc grace period (which is 2 weeks by default). So object themselves _can_ be a source of traversal for git-gc. We do that traversal with the ignore_missing_links flag, so breakages in the unreachable objects _shouldn't_ cause what you're seeing. IIRC we did turn up a bug or two with ignore_missing_links. The only one I could find was a3ba6bf10a (revision.c: ignore broken tags with ignore_missing_links, 2017-05-20), which I think wouldn't generate the output you're seeing.
Recovering from gc errors
(I'm using git 2.15.0.) So today "git gc" started complaining: error: Could not read 2bc277bcb7e9cc6ef2ea677dd1c3dcd1f9af0c2b fatal: Failed to traverse parents of commit 9c355a7726e31b3033b8e714cf7edb4f0a41d8d4 error: failed to run repack I suspect I'm a victim of the worktree+submodule bugs -- as a longtime user of contrib/workdir/git-new-workdir, I've been playing with the "worktree" command since it was first introduced. The "git gc" error occurs when it's run in my main repo; I have not tried it in any of my worktrees/workdirs. Various incantations of "git show ... 9c355a7726e31" only fail with the same error, so I can't determine much about the problematic commit. Luckily I'm not particularly concerned with losing objects, as I push any important progress to named refs in backup repos. But I would like to clean this up in my local repo so that gc stops failing. I tried simply removing this and other loose commits that trip up gc (i.e. the objects/9c/355a7726e31b3033b8e714cf7edb4f0a41d8d4 file -- there are 49 such files, all of which are several months old), but now gc complains of a bad tree object: error: Could not read c1a99c3520f0b456b8025c50302a4cc9b0b2d777 fatal: bad tree object c1a99c3520f0b456b8025c50302a4cc9b0b2d777 error: failed to run repack This object is not lying around loose. "git fsck" lists several dangling blob/commit/tree objects, but none of them are c1a99c3520f0b4. So I'm not sure what to do next. Any suggestions? Thanks, M.
Re: [RFC PATCH] proposal for refs/tracking/* hierarchy
On 2017-06-23 04:54 PM, Junio C Hamano wrote: Jacob Keller writes: Instead, lets add support for a new refs/tracking/* hierarchy which is laid out in such a way to avoid this inconsistency. All refs in "refs/tracking//*" will include the complete ref, such that dropping the "tracking/" part will give the exact ref name as it is found in the upstream. Thus, we can track any ref here by simply fetching it into refs/tracking//*. I do think this overall is a good goal to aim for wrt "tracking", i.e. keeping a pristine copy of what we observed from the outside world. In addition to what you listed as "undecided" below, however, I expect that this will highlight a lot of trouble in "working together", i.e. reconciling the differences of various world views and moving the project forward, in two orthogonal axes: - Things pointed at by some refs (e.g. notes/, but I think the ".gitmodules equivalent that is not tied to any particular commit in the superproject" Jonathan Nieder and friends advocate falls into the same category) do not correspond to the working tree contents, and merging their contents will always pose challenge when we need to prepare for conflict resolution. OTOH, we shouldn't need to do any conflict resolution on these "tracking" refs: The remote side should update the refs properly. Nobody should make local changes to these refs. I guess I'm advocating that git should only allow "tracking" refs to be updated by a fetch, or a successful push of a local, non-"tracking" ref. M. - Things pointed at by some other refs (e.g. tags/) do not make sense to be merged. You may locally call a commit with a tag "wip", while your friends may use the same "wip" tag to point at a different one. Both are valid, and more importantly, there is no point even trying to reconcile what the "wip" tag means in the project globally. For the former, I expect that merging these non-working tree contents will all have to require some specific tool that is tailored for the meaning each hierarchy has, just like "git notes merge" gives a solution that is very specific to the notes refs (I do not know how well "notes merge" works in practice, though). For the latter, having a separate tracking hierarchy is a strict improvement from "tracking" point of view, but I think "working together" also needs a well designed set of new look-up rules, and a new convention. For example, some tags may not want to be shared (e.g. "wip" example above) even though they should be easy to access by those who already have them (e.g. "git log ..wip" should work without having to say "git log ..refs/local-tags/wip", even if we decide to implement the "some tags may not want to be shared" segregation by introducing a new hierarchy). And also a shared tag that is copied to "refs/tracking/origin/tags/v1.0" would need a way better than "git log tracking/origin/tags/v1.0" for this mechanism to be useful in everyday workflow. Thanks.
[PATCHv2 (resend)] Tweak help auto-correct phrasing.
When auto-correct is enabled, an invalid git command prints a warning and a continuation message, which differs depending on whether or not help.autoCorrect is positive or negative. With help.autoCorrect = 15: WARNING: You called a Git command named 'lgo', which does not exist. Continuing under the assumption that you meant 'log' in 1.5 seconds automatically... With help.autoCorrect < 0: WARNING: You called a Git command named 'lgo', which does not exist. Continuing under the assumption that you meant 'log' The continuation message's phrasing is awkward. This commit cleans it up. As a bonus, we now use full-sentence strings which make translation easier. With help.autoCorrect = 15: WARNING: You called a Git command named 'lgo', which does not exist. Continuing in 1.5 seconds, assuming that you meant 'log'. With help.autoCorrect < 0: WARNING: You called a Git command named 'lgo', which does not exist. Continuing under the assumption that you meant 'log'. Signed-off-by: Marc Branchaud --- So here's the patch again. M. help.c | 18 -- 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/help.c b/help.c index f637fc800..69966c174 100644 --- a/help.c +++ b/help.c @@ -356,12 +356,18 @@ const char *help_unknown_cmd(const char *cmd) clean_cmdnames(&main_cmds); fprintf_ln(stderr, _("WARNING: You called a Git command named '%s', " -"which does not exist.\n" -"Continuing under the assumption that you meant '%s'"), - cmd, assumed); - if (autocorrect > 0) { - fprintf_ln(stderr, _("in %0.1f seconds automatically..."), - (float)autocorrect/10.0); +"which does not exist."), + cmd); + if (autocorrect < 0) + fprintf_ln(stderr, + _("Continuing under the assumption that " +"you meant '%s'."), + assumed); + else { + fprintf_ln(stderr, + _("Continuing in %0.1f seconds, " +"assuming that you meant '%s'."), + (float)autocorrect/10.0, assumed); sleep_millisec(autocorrect * 100); } return assumed; -- 2.13.1.388.g69e6b9b4f.dirty
Re: [PATCHv2] Tweak help auto-correct phrasing.
On 2017-06-20 02:04 PM, Kaartic Sivaraam wrote: On Tue, 2016-12-20 at 09:02 -0500, Marc Branchaud wrote: When auto-correct is enabled, an invalid git command prints a warning and a continuation message, which differs depending on whether or not help.autoCorrect is positive or negative. With help.autoCorrect = 15: WARNING: You called a Git command named 'lgo', which does not exist. Continuing under the assumption that you meant 'log' in 1.5 seconds automatically... With help.autoCorrect < 0: WARNING: You called a Git command named 'lgo', which does not exist. Continuing under the assumption that you meant 'log' The continuation message's phrasing is awkward. This commit cleans it up. As a bonus, we now use full-sentence strings which make translation easier. With help.autoCorrect = 15: WARNING: You called a Git command named 'lgo', which does not exist. Continuing in 1.5 seconds, assuming that you meant 'log'. With help.autoCorrect < 0: WARNING: You called a Git command named 'lgo', which does not exist. Continuing under the assumption that you meant 'log'. Signed-off-by: Marc Branchaud --- Excuse me for bringing this up after a long time. What's the status of this patch? Was it applied? Looks like it got lost in the shuffle. The topic thread starts at: http://public-inbox.org/git/1482063500.10858.1.ca...@gmail.com/ There's no reply to my v2 patch, and I neglected to follow up on it -- sorry! Shall I resend the patch? M.
Re: proposal for how to share other refs as part of refs/tracking/*
On 2017-06-13 10:41 AM, Marc Branchaud wrote: So I like your refs/tracking proposal, and hope that it aims for mirroring a remote's refs, to eventually replace refs/remotes entirely. To be extra-clear: I think a refs/tracking hierarchy that starts with notes and maybe some other bits is a good first step. I *hope* such a first step can eventually mirror all of a remote's refs, including heads and tags. I in no way think that this effort should begin by tackling heads and tags. M.
Re: proposal for how to share other refs as part of refs/tracking/*
On 2017-06-12 06:58 PM, Jacob Keller wrote: Hi, There's no actual code yet, (forgive me), but I've been thinking back to a while ago about attempting to find a way to share things like refs/notes, and similar refs which are usually not shared across a remote. By default, those refs are not propagated when you do a push or a pull, and this makes using them in any project which has more then one repository quite difficult. I'm going to focus the discussion primarily on refs/notes as this is what I am most interested in, but I think similar issues exist for refs/grafts and refs/replace, and maybe other sources? More formal support for custom ref namespaces would be a boon. For example, we have our nightly builds create a "build/w.x.y-z" ref (we only want to tag official releases). Sharing those refs is not hard, but a bit obscure. For branches, we already have a system to share the status of remote branches, called "refs/remotes//" This hierarchy unfortunately does not keep space for non-branches, because you can't simply add a "refs/remotes/notes/<>" or "refs/remotes//notes" to this as it would be ambiguous. Now, you might just decide to push the refs/notes directly, ie: git push origin refs/notes/...:refs/notes/... Unfortunately, this has problems because it leaves no standard way to distinguish your local work from what is on the remote, so you can't easily merge the remote work into yours. There was a related discussion in the run-up to 1.8.0, about a "remote tag namespace" to support having different remotes with the same tag name for different objects. See these messages and their surrounding threads: http://public-inbox.org/git/AANLkTikeqsg+qJ0z4iQ6ZmKL=_HB8YX_z20L=dffa...@mail.gmail.com/ http://public-inbox.org/git/AANLkTi=yfwoaqmhhvlsb1_xmyoe9hhp2yb4h4tqzw...@mail.gmail.com/ http://public-inbox.org/git/201102020322.00171.jo...@herland.net/ The discussion explored, among other things, making refs/remotes/$remote/* a mirror of a remote's own refs/* hierarchy (well, within reason -- I think there are limits to what should be mirrored). So I like your refs/tracking proposal, and hope that it aims for mirroring a remote's refs, to eventually replace refs/remotes entirely. M. For example, if Alice creates a new note and pushes it, then Bob creates a different note on the same commit, he needs to be able to merge Alice's changes into his note, just like one would do when two people commit to the same branch. Today, he must pull the remote notes into a separate location, (since pulling directly into refs/notes will overwrite his work), and then update, and then push. Because this is not standardized, it becomes unwieldy to actually perform on a day to day basis. I propose that we add a new "refs/tracking" hierarchy which will be used to track these type of refs We could even (long term) migrate refs/remotes into refs/tracking instead, or provide both with the refs/remotes being pointers or something like that.. Essentially, refs/notes would be pulled into refs/tracking//notes/* or something along these lines. Then, git notes would be modified to be able to have commands to "pull" and "push" notes which would fetch the remote, and then attempt a merge into the local notes, while a push would update the remote. I chose "tracking" because it sort of fits the concept and does not include things like "remote-notes" or "remotes-v2" which are a bit weird. I'm posting this on the mailing list prior to having code because I wanted to get a sense of how people felt about the question and whether others still felt it was an issue. Essentially the goal is to standardize how any refs namespace can be shared in such a way that local copies can tell what the remote had at a fetch time, in order to allow easier handling of conflicts between local and remote changes, just like we do for branches (heads) but generalized so that other refs namespaces can get the same ability to handle conflicts. Thanks, Jake
Re: [PATCH] tag: duplicate mention of --contains should mention --no-contains
On 2017-05-15 11:01 AM, Ævar Arnfjörð Bjarmason wrote: On Mon, May 15, 2017 at 4:20 PM, Marc Branchaud wrote: On 2017-05-15 08:23 AM, Ævar Arnfjörð Bjarmason wrote: Fix a duplicate mention of --contains in the SYNOPSIS to mention --no-contains. This fixes an error introduced in my commit ac3f5a3468 ("ref-filter: add --no-contains option to tag/branch/for-each-ref", 2017-03-24). Signed-off-by: Ævar Arnfjörð Bjarmason --- Documentation/git-tag.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt index f8a0b787f4..1eb15afa1c 100644 --- a/Documentation/git-tag.txt +++ b/Documentation/git-tag.txt @@ -12,7 +12,7 @@ SYNOPSIS 'git tag' [-a | -s | -u ] [-f] [-m | -F ] [ | ] 'git tag' -d ... -'git tag' [-n[]] -l [--contains ] [--contains ] +'git tag' [-n[]] -l [--contains ] [--no-contains ] I think [--[no-]contains ] is the usual pattern... [--points-at ] [--column[=] | --no-column] [--create-reflog] [--sort=] [--format=] [--[no-]merged []] [...] ... like with --[no-]merged, there. M. Thanks for the feedback, this was discussed earlier in the series and we decided on the current format I'm submitting here. Saying --[no-]merged is the convention we use for options where the two are mutually exclusive, as is the case with the --[no-]merged options: $ git tag --merged v2.12.0 --no-merged v2.13.0 error: option `no-merged' is incompatible with --merged [...] But in the case of --contains and --no-contains you can provide both: $ git tag --contains v2.12.0 --no-contains v2.13.0 'v*' v2.12.0 v2.12.1 v2.12.2 v2.12.3 v2.13.0-rc0 v2.13.0-rc1 v2.13.0-rc2 So they don't use that convention, since it would imply that they're mutually exclusive, rather than both being optional. Ah, thanks. My mistake! M.
Re: [PATCH] tag: duplicate mention of --contains should mention --no-contains
On 2017-05-15 08:23 AM, Ævar Arnfjörð Bjarmason wrote: Fix a duplicate mention of --contains in the SYNOPSIS to mention --no-contains. This fixes an error introduced in my commit ac3f5a3468 ("ref-filter: add --no-contains option to tag/branch/for-each-ref", 2017-03-24). Signed-off-by: Ævar Arnfjörð Bjarmason --- Documentation/git-tag.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt index f8a0b787f4..1eb15afa1c 100644 --- a/Documentation/git-tag.txt +++ b/Documentation/git-tag.txt @@ -12,7 +12,7 @@ SYNOPSIS 'git tag' [-a | -s | -u ] [-f] [-m | -F ] [ | ] 'git tag' -d ... -'git tag' [-n[]] -l [--contains ] [--contains ] +'git tag' [-n[]] -l [--contains ] [--no-contains ] I think [--[no-]contains ] is the usual pattern... [--points-at ] [--column[=] | --no-column] [--create-reflog] [--sort=] [--format=] [--[no-]merged []] [...] ... like with --[no-]merged, there. M.
Re: What's cooking in git.git (May 2017, #03; Wed, 10)
On 2017-05-10 01:18 AM, Junio C Hamano wrote: * mb/diff-default-to-indent-heuristics (2017-05-09) 4 commits - add--interactive: drop diff.indentHeuristic handling - diff: enable indent heuristic by default - diff: have the diff-* builtins configure diff before initializing revisions - diff: make the indent heuristic part of diff's basic configuration Make the "indent" heuristics the default in "diff" and diff.indentHeuristics configuration variable an escape hatch for those who do no want it. Typo fixes: s/heuristics/heuristic/ (both places) s/do no want/do not want/ Kicked out of next; it seems it is still getting review suggestions? I believe v4 of this one is ready to cook. The most salient aspect of the review discussion was about where to go after this topic is applied. We also concluded that the topic deserves a release note about breaking patch ID backwards-compatibility. I think such a note should mention rerere, so I would suggest the following: The diff "indent" heuristic is now enabled by default. This changes the patch IDs calculated by git-patch-id (and used by git-rerere and git-cherry), which could affect workflows that rely on previously-computed patch IDs. The heuristic can be disabled by setting diff.indentHeuristic to false. M.
Re: [PATCH v4 2/4] diff: have the diff-* builtins configure diff before initializing revisions
On 2017-05-08 11:22 PM, Jeff King wrote: On Mon, May 08, 2017 at 12:03:37PM -0400, Marc Branchaud wrote: This matches how the diff Porcelain works. It makes the plumbing commands respect diff's configuration options, such as indentHeuristic, because init_revisions() calls diff_setup() which fills in the diff_options struct. I don't know if you want to note here that this is only _some_ options. I.e., ones that we handle by copying via diff_setup(). Maybe it's obvious from the description already (it's hard for me to tell because I already know it either way :) ). (shrug) I'm fine with the way it is, but I'd also be OK with "respect some of diff's configuration options". Junio, please feel free to reword the message if you like. Or I can send out a v5, if that's easier for you. M.
[PATCH v4 1/4] diff: make the indent heuristic part of diff's basic configuration
This heuristic was originally introduced as an experimental feature, and therefore part of the UI configuration. But the user often sees diffs generated by plumbing commands like diff-tree. Moving the indent heuristic into diff's basic configuration prepares the way for diff plumbing commands to respect the setting. The heuristic itself merely makes the diffs more aesthetically pleasing, without changing their correctness. Scripts that rely on the diff plumbing commands should not care whether or not the heuristic is employed. Signed-off-by: Marc Branchaud --- diff.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/diff.c b/diff.c index 74283d900..b6e3ffe92 100644 --- a/diff.c +++ b/diff.c @@ -290,9 +290,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb) return 0; } - if (git_diff_heuristic_config(var, value, cb) < 0) - return -1; - if (!strcmp(var, "diff.wserrorhighlight")) { int val = parse_ws_error_highlight(value); if (val < 0) @@ -351,6 +348,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb) if (starts_with(var, "submodule.")) return parse_submodule_config_option(var, value); + if (git_diff_heuristic_config(var, value, cb) < 0) + return -1; + return git_default_config(var, value, cb); } -- 2.13.0.rc1.15.gf67d331ad
[PATCH v4 2/4] diff: have the diff-* builtins configure diff before initializing revisions
This matches how the diff Porcelain works. It makes the plumbing commands respect diff's configuration options, such as indentHeuristic, because init_revisions() calls diff_setup() which fills in the diff_options struct. Signed-off-by: Marc Branchaud --- builtin/diff-files.c | 2 +- builtin/diff-index.c | 2 +- builtin/diff-tree.c| 2 +- t/t4061-diff-indent.sh | 66 ++ 4 files changed, 69 insertions(+), 3 deletions(-) diff --git a/builtin/diff-files.c b/builtin/diff-files.c index 15c61fd8d..a572da9d5 100644 --- a/builtin/diff-files.c +++ b/builtin/diff-files.c @@ -20,9 +20,9 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix) int result; unsigned options = 0; + git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ init_revisions(&rev, prefix); gitmodules_config(); - git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ rev.abbrev = 0; precompose_argv(argc, argv); diff --git a/builtin/diff-index.c b/builtin/diff-index.c index 1af373d00..f084826a2 100644 --- a/builtin/diff-index.c +++ b/builtin/diff-index.c @@ -17,9 +17,9 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) int i; int result; + git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ init_revisions(&rev, prefix); gitmodules_config(); - git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ rev.abbrev = 0; precompose_argv(argc, argv); diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c index 326f88b65..36a3a1976 100644 --- a/builtin/diff-tree.c +++ b/builtin/diff-tree.c @@ -105,9 +105,9 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix) struct setup_revision_opt s_r_opt; int read_stdin = 0; + git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ init_revisions(opt, prefix); gitmodules_config(); - git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ opt->abbrev = 0; opt->diff = 1; opt->disable_stdin = 1; diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh index 556450609..13d3dc96a 100755 --- a/t/t4061-diff-indent.sh +++ b/t/t4061-diff-indent.sh @@ -213,4 +213,70 @@ test_expect_success 'blame: --no-indent-heuristic overrides config' ' compare_blame spaces-expect out-blame2 ' +test_expect_success 'diff-tree: nice spaces with --indent-heuristic' ' + git diff-tree --indent-heuristic -p old new -- spaces.txt >out-diff-tree-compacted && + compare_diff spaces-compacted-expect out-diff-tree-compacted +' + +test_expect_success 'diff-tree: nice spaces with diff.indentHeuristic' ' + git -c diff.indentHeuristic=true diff-tree -p old new -- spaces.txt >out-diff-tree-compacted2 && + compare_diff spaces-compacted-expect out-diff-tree-compacted2 +' + +test_expect_success 'diff-tree: --no-indent-heuristic overrides config' ' + git -c diff.indentHeuristic=true diff-tree --no-indent-heuristic -p old new -- spaces.txt >out-diff-tree && + compare_diff spaces-expect out-diff-tree +' + +test_expect_success 'diff-index: nice spaces with --indent-heuristic' ' + git checkout -B diff-index && + git reset --soft HEAD~ && + git diff-index --indent-heuristic -p old -- spaces.txt >out-diff-index-compacted && + compare_diff spaces-compacted-expect out-diff-index-compacted && + git checkout -f master +' + +test_expect_success 'diff-index: nice spaces with diff.indentHeuristic' ' + git checkout -B diff-index && + git reset --soft HEAD~ && + git -c diff.indentHeuristic=true diff-index -p old -- spaces.txt >out-diff-index-compacted2 && + compare_diff spaces-compacted-expect out-diff-index-compacted2 && + git checkout -f master +' + +test_expect_success 'diff-index: --no-indent-heuristic overrides config' ' + git checkout -B diff-index && + git reset --soft HEAD~ && + git -c diff.indentHeuristic=true diff-index --no-indent-heuristic -p old -- spaces.txt >out-diff-index && + compare_diff spaces-expect out-diff-index && + git checkout -f master +' + +test_expect_success 'diff-files: nice spaces with diff.indentHeuristic' ' + git checkout -B diff-files && + git reset HEAD~ && + git -c diff.indentHeuristic=true diff-files -p spaces.txt >out-diff-files-raw && + grep -v index out-diff-files-raw >out-diff-files
[PATCH v4 3/4] diff: enable indent heuristic by default
From: Stefan Beller The feature was included in v2.11 (released 2016-11-29) and we got no negative feedback. Quite the opposite, all feedback we got was positive. Turn it on by default. Users who dislike the feature can turn it off by setting diff.indentHeuristic (which also configures plumbing commands, see prior patches). The change to t/t4051-diff-function-context.sh is needed because the heuristic shifts the changed hunk in the patch. To get the same result regardless of the heuristic configuration, we modify the test file differently: We insert a completely new line after line 2, instead of simply duplicating it. Helped-by: Jeff King Signed-off-by: Stefan Beller Signed-off-by: Marc Branchaud --- diff.c | 2 +- t/t4051-diff-function-context.sh | 3 +- t/t4061-diff-indent.sh | 140 +++ 3 files changed, 116 insertions(+), 29 deletions(-) diff --git a/diff.c b/diff.c index b6e3ffe92..a24452051 100644 --- a/diff.c +++ b/diff.c @@ -27,7 +27,7 @@ #endif static int diff_detect_rename_default; -static int diff_indent_heuristic; /* experimental */ +static int diff_indent_heuristic = 1; static int diff_rename_limit_default = 400; static int diff_suppress_blank_empty; static int diff_use_color_default = -1; diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh index 6154acb45..3e6b485ec 100755 --- a/t/t4051-diff-function-context.sh +++ b/t/t4051-diff-function-context.sh @@ -72,7 +72,8 @@ test_expect_success 'setup' ' # overlap function context of 1st change and -u context of 2nd change grep -v "delete me from hello" <"$dir/hello.c" >file.c && - sed 2p <"$dir/dummy.c" >>file.c && + sed "2a\\ +extra line" <"$dir/dummy.c" >>file.c && commit_and_tag changed_hello_dummy file.c && git checkout initial && diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh index 13d3dc96a..2affd7a10 100755 --- a/t/t4061-diff-indent.sh +++ b/t/t4061-diff-indent.sh @@ -152,26 +152,28 @@ test_expect_success 'prepare' ' EOF ' +# --- diff tests -- + test_expect_success 'diff: ugly spaces' ' - git diff old new -- spaces.txt >out && + git diff --no-indent-heuristic old new -- spaces.txt >out && compare_diff spaces-expect out ' +test_expect_success 'diff: --no-indent-heuristic overrides config' ' + git -c diff.indentHeuristic=true diff --no-indent-heuristic old new -- spaces.txt >out2 && + compare_diff spaces-expect out2 +' + test_expect_success 'diff: nice spaces with --indent-heuristic' ' - git diff --indent-heuristic old new -- spaces.txt >out-compacted && + git -c diff.indentHeuristic=false diff --indent-heuristic old new -- spaces.txt >out-compacted && compare_diff spaces-compacted-expect out-compacted ' -test_expect_success 'diff: nice spaces with diff.indentHeuristic' ' +test_expect_success 'diff: nice spaces with diff.indentHeuristic=true' ' git -c diff.indentHeuristic=true diff old new -- spaces.txt >out-compacted2 && compare_diff spaces-compacted-expect out-compacted2 ' -test_expect_success 'diff: --no-indent-heuristic overrides config' ' - git -c diff.indentHeuristic=true diff --no-indent-heuristic old new -- spaces.txt >out2 && - compare_diff spaces-expect out2 -' - test_expect_success 'diff: --indent-heuristic with --patience' ' git diff --indent-heuristic --patience old new -- spaces.txt >out-compacted3 && compare_diff spaces-compacted-expect out-compacted3 @@ -183,7 +185,7 @@ test_expect_success 'diff: --indent-heuristic with --histogram' ' ' test_expect_success 'diff: ugly functions' ' - git diff old new -- functions.c >out && + git diff --no-indent-heuristic old new -- functions.c >out && compare_diff functions-expect out ' @@ -192,42 +194,73 @@ test_expect_success 'diff: nice functions with --indent-heuristic' ' compare_diff functions-compacted-expect out-compacted ' -test_expect_success 'blame: ugly spaces' ' - git blame old..new -- spaces.txt >out-blame && - compare_blame spaces-expect out-blame -' +# --- blame tests - test_expect_success 'blame: nice spaces with --indent-heuristic' ' git blame --indent-heuristic old..new -- spaces.txt >out-blame
[PATCH v4 4/4] add--interactive: drop diff.indentHeuristic handling
From: Jeff King Now that diff.indentHeuristic is handled automatically by the plumbing commands, there's no need to propagate it manually. Signed-off-by: Jeff King Signed-off-by: Marc Branchaud --- git-add--interactive.perl | 4 1 file changed, 4 deletions(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 709a5f6ce..79d675b5b 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -46,7 +46,6 @@ my ($diff_new_color) = my $normal_color = $repo->get_color("", "reset"); my $diff_algorithm = $repo->config('diff.algorithm'); -my $diff_indent_heuristic = $repo->config_bool('diff.indentheuristic'); my $diff_filter = $repo->config('interactive.difffilter'); my $use_readkey = 0; @@ -730,9 +729,6 @@ sub parse_diff { if (defined $diff_algorithm) { splice @diff_cmd, 1, 0, "--diff-algorithm=${diff_algorithm}"; } - if ($diff_indent_heuristic) { - splice @diff_cmd, 1, 0, "--indent-heuristic"; - } if (defined $patch_mode_revision) { push @diff_cmd, get_diff_reference($patch_mode_revision); } -- 2.13.0.rc1.15.gf67d331ad
[PATCH v4 0/4] Make diff plumbing commands respect the indentHeuristic.
The only change from v3 is in 3/4, to expand t4061 to test various combinations of --(no-)indent-heuristic and diff.indentHeuristic. I kindof went all-in and tried to cover every possible combination for all four affected commands. An inter-diff is below. M. Jeff King (1): add--interactive: drop diff.indentHeuristic handling Marc Branchaud (2): diff: make the indent heuristic part of diff's basic configuration diff: have the diff-* builtins configure diff before initializing revisions Stefan Beller (1): diff: enable indent heuristic by default builtin/diff-files.c | 2 +- builtin/diff-index.c | 2 +- builtin/diff-tree.c | 2 +- diff.c | 8 +- git-add--interactive.perl| 4 - t/t4051-diff-function-context.sh | 3 +- t/t4061-diff-indent.sh | 184 +++ 7 files changed, 177 insertions(+), 28 deletions(-) diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh index 56d7d7760..2affd7a10 100755 --- a/t/t4061-diff-indent.sh +++ b/t/t4061-diff-indent.sh @@ -152,26 +152,28 @@ test_expect_success 'prepare' ' EOF ' +# --- diff tests -- + test_expect_success 'diff: ugly spaces' ' git diff --no-indent-heuristic old new -- spaces.txt >out && compare_diff spaces-expect out ' +test_expect_success 'diff: --no-indent-heuristic overrides config' ' + git -c diff.indentHeuristic=true diff --no-indent-heuristic old new -- spaces.txt >out2 && + compare_diff spaces-expect out2 +' + test_expect_success 'diff: nice spaces with --indent-heuristic' ' - git diff --indent-heuristic old new -- spaces.txt >out-compacted && + git -c diff.indentHeuristic=false diff --indent-heuristic old new -- spaces.txt >out-compacted && compare_diff spaces-compacted-expect out-compacted ' -test_expect_success 'diff: nice spaces with diff.indentHeuristic' ' +test_expect_success 'diff: nice spaces with diff.indentHeuristic=true' ' git -c diff.indentHeuristic=true diff old new -- spaces.txt >out-compacted2 && compare_diff spaces-compacted-expect out-compacted2 ' -test_expect_success 'diff: --no-indent-heuristic overrides config' ' - git -c diff.indentHeuristic=true diff --no-indent-heuristic old new -- spaces.txt >out2 && - compare_diff spaces-expect out2 -' - test_expect_success 'diff: --indent-heuristic with --patience' ' git diff --indent-heuristic --patience old new -- spaces.txt >out-compacted3 && compare_diff spaces-compacted-expect out-compacted3 @@ -192,42 +194,73 @@ test_expect_success 'diff: nice functions with --indent-heuristic' ' compare_diff functions-compacted-expect out-compacted ' -test_expect_success 'blame: ugly spaces' ' - git blame --no-indent-heuristic old..new -- spaces.txt >out-blame && - compare_blame spaces-expect out-blame -' +# --- blame tests - test_expect_success 'blame: nice spaces with --indent-heuristic' ' git blame --indent-heuristic old..new -- spaces.txt >out-blame-compacted && compare_blame spaces-compacted-expect out-blame-compacted ' -test_expect_success 'blame: nice spaces with diff.indentHeuristic' ' +test_expect_success 'blame: nice spaces with diff.indentHeuristic=true' ' git -c diff.indentHeuristic=true blame old..new -- spaces.txt >out-blame-compacted2 && compare_blame spaces-compacted-expect out-blame-compacted2 ' +test_expect_success 'blame: ugly spaces with --no-indent-heuristic' ' + git blame --no-indent-heuristic old..new -- spaces.txt >out-blame && + compare_blame spaces-expect out-blame +' + +test_expect_success 'blame: ugly spaces with diff.indentHeuristic=false' ' + git -c diff.indentHeuristic=false blame old..new -- spaces.txt >out-blame2 && + compare_blame spaces-expect out-blame2 +' + test_expect_success 'blame: --no-indent-heuristic overrides config' ' - git -c diff.indentHeuristic=true blame --no-indent-heuristic old..new -- spaces.txt >out-blame2 && + git -c diff.indentHeuristic=true blame --no-indent-heuristic old..new -- spaces.txt >out-blame3 && git blame old..new -- spaces.txt >out-blame && - compare_blame spaces-expect out-blame2 + compare_blame spaces-expect out-blame3 ' +test_expect_success
Enabling the diff "indent" heuristic by default
On 2017-05-08 03:48 AM, Junio C Hamano wrote: * mb/diff-default-to-indent-heuristics (2017-05-02) 4 commits (merged to 'next' on 2017-05-08 at 158f401a92) I think there's a general open question about this, which is whether or not we should just drop the diff.indentHeuristic configuration setting altogether. Peff made the point [0] that if we keep the setting then t4061 should be rewritten. My instinct is to keep the setting, at least until the changed default has a bit of time to settle in. So I'll re-send the topic with the renovated t4061. The topic would of course change more drastically if we decide to drop the setting right away. + add--interactive: drop diff.indentHeuristic handling + diff: enable indent heuristic by default + diff: have the diff-* builtins configure diff before initializing revisions + diff: make the indent heuristic part of diff's basic configuration Make the "indent" heuristics the default in "diff" and diff.indentHeuristics s/heuristics/heuristic/ (both places) configuration variable an escape hatch for those who do no want it. s/do no/do not/ Will cook in 'next'. Both Peff [1] and Ævar [2] mentioned situations where enabling the heuristic has a small impact on them. If/when this graduates, it's perhaps worth adding a backward-compatibility note that the default patch IDs are changing. Maybe something like: The diff "indent" heuristic is now enabled by default. This changes the patch IDs calculated by git-patch-id and used by git-cherry, which could affect patch-based workflows that rely on previously-computed patch IDs. The heuristic can be disabled by setting diff.indentHeuristic to false. [0] https://public-inbox.org/git/20170501222051.svylxazjwnot3...@sigill.intra.peff.net/ [1] https://public-inbox.org/git/20170428220450.olqitnuwhrxzg...@sigill.intra.peff.net/ [2] https://public-inbox.org/git/cacbzzx5f81hkcjrjtdyxznmvuef9z_ecs+0svk2xpbwxudg...@mail.gmail.com/ M.
[PATCHv3 3/4] diff: enable indent heuristic by default
From: Stefan Beller The feature was included in v2.11 (released 2016-11-29) and we got no negative feedback. Quite the opposite, all feedback we got was positive. Turn it on by default. Users who dislike the feature can turn it off by setting diff.indentHeuristic (which also configures plumbing commands, see prior patches). The change to t/t4051-diff-function-context.sh is needed because the heuristic shifts the changed hunk in the patch. To get the same result regardless of the heuristic configuration, we modify the test file differently: We insert a completely new line after line 2, instead of simply duplicating it. Helped-by: Jeff King Signed-off-by: Stefan Beller Signed-off-by: Marc Branchaud --- Tested the sed "2a" command's escaping in both Ubuntu 17.04 and FreeBSD 10.3. Threw in a little indenting so that it isn't too ugly. diff.c | 2 +- t/t4051-diff-function-context.sh | 3 ++- t/t4061-diff-indent.sh | 6 +++--- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/diff.c b/diff.c index da96577ea..2c47ccb4a 100644 --- a/diff.c +++ b/diff.c @@ -27,7 +27,7 @@ #endif static int diff_detect_rename_default; -static int diff_indent_heuristic; /* experimental */ +static int diff_indent_heuristic = 1; static int diff_rename_limit_default = 400; static int diff_suppress_blank_empty; static int diff_use_color_default = -1; diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh index 6154acb45..3e6b485ec 100755 --- a/t/t4051-diff-function-context.sh +++ b/t/t4051-diff-function-context.sh @@ -72,7 +72,8 @@ test_expect_success 'setup' ' # overlap function context of 1st change and -u context of 2nd change grep -v "delete me from hello" <"$dir/hello.c" >file.c && - sed 2p <"$dir/dummy.c" >>file.c && + sed "2a\\ +extra line" <"$dir/dummy.c" >>file.c && commit_and_tag changed_hello_dummy file.c && git checkout initial && diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh index 13d3dc96a..56d7d7760 100755 --- a/t/t4061-diff-indent.sh +++ b/t/t4061-diff-indent.sh @@ -153,7 +153,7 @@ test_expect_success 'prepare' ' ' test_expect_success 'diff: ugly spaces' ' - git diff old new -- spaces.txt >out && + git diff --no-indent-heuristic old new -- spaces.txt >out && compare_diff spaces-expect out ' @@ -183,7 +183,7 @@ test_expect_success 'diff: --indent-heuristic with --histogram' ' ' test_expect_success 'diff: ugly functions' ' - git diff old new -- functions.c >out && + git diff --no-indent-heuristic old new -- functions.c >out && compare_diff functions-expect out ' @@ -193,7 +193,7 @@ test_expect_success 'diff: nice functions with --indent-heuristic' ' ' test_expect_success 'blame: ugly spaces' ' - git blame old..new -- spaces.txt >out-blame && + git blame --no-indent-heuristic old..new -- spaces.txt >out-blame && compare_blame spaces-expect out-blame ' -- 2.13.0.rc1.15.gf67d331ad
[PATCHv3 4/4] add--interactive: drop diff.indentHeuristic handling
From: Jeff King Now that diff.indentHeuristic is handled automatically by the plumbing commands, there's no need to propagate it manually. Signed-off-by: Jeff King Signed-off-by: Marc Branchaud --- git-add--interactive.perl | 4 1 file changed, 4 deletions(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 709a5f6ce..79d675b5b 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -46,7 +46,6 @@ my ($diff_new_color) = my $normal_color = $repo->get_color("", "reset"); my $diff_algorithm = $repo->config('diff.algorithm'); -my $diff_indent_heuristic = $repo->config_bool('diff.indentheuristic'); my $diff_filter = $repo->config('interactive.difffilter'); my $use_readkey = 0; @@ -730,9 +729,6 @@ sub parse_diff { if (defined $diff_algorithm) { splice @diff_cmd, 1, 0, "--diff-algorithm=${diff_algorithm}"; } - if ($diff_indent_heuristic) { - splice @diff_cmd, 1, 0, "--indent-heuristic"; - } if (defined $patch_mode_revision) { push @diff_cmd, get_diff_reference($patch_mode_revision); } -- 2.13.0.rc1.15.gf67d331ad
[PATCHv3 0/4] Make diff plumbing commands respect the indentHeuristic.
On 2017-04-29 09:14 AM, Jeff King wrote: > On Sat, Apr 29, 2017 at 08:40:52AM -0400, Jeff King wrote: > >> On Fri, Apr 28, 2017 at 06:33:12PM -0400, Marc Branchaud wrote: >> >>> v2: Fixed up the commit messages and added tests. >>> >>> Marc Branchaud (2): >>> diff: make the indent heuristic part of diff's basic configuration >>> diff: have the diff-* builtins configure diff before initializing >>> revisions >>> >>> Stefan Beller (1): >>> diff: enable indent heuristic by default >> >> Thanks, these look fine to me. I'd like to get an ACK from Michael, in >> case he had some other reason for omitting them from git_diff_ui_config >> (from my recollection, it's probably just a mix of conservatism and >> following what the compaction heuristic had done). > > Sorry, I spoke too soon. The third one needs a few test adjustments > squashed in to pass the tests. Doh! That'll teach me to try to do this stuff at the end of a Friday... One more try, then: Changes since v2: Patch 1/4 : Unchanged. Patch 2/4 : Mentioned how the new behaviour matches the diff Porcelain. Patch 3/4 : Updated the tests. Patch 4/4 : (New) Jeff's add--interactive patch. Thanks for all the help, Jeff! M. Jeff King (1): add--interactive: drop diff.indentHeuristic handling Marc Branchaud (2): diff: make the indent heuristic part of diff's basic configuration diff: have the diff-* builtins configure diff before initializing revisions Stefan Beller (1): diff: enable indent heuristic by default builtin/diff-files.c | 2 +- builtin/diff-index.c | 2 +- builtin/diff-tree.c | 2 +- diff.c | 8 ++--- git-add--interactive.perl| 4 --- t/t4051-diff-function-context.sh | 3 +- t/t4061-diff-indent.sh | 72 ++-- 7 files changed, 78 insertions(+), 15 deletions(-) -- 2.13.0.rc1.15.gf67d331ad
[PATCHv3 1/4] diff: make the indent heuristic part of diff's basic configuration
This heuristic was originally introduced as an experimental feature, and therefore part of the UI configuration. But the user often sees diffs generated by plumbing commands like diff-tree. Moving the indent heuristic into diff's basic configuration prepares the way for diff plumbing commands to respect the setting. The heuristic itself merely makes the diffs more aesthetically pleasing, without changing their correctness. Scripts that rely on the diff plumbing commands should not care whether or not the heuristic is employed. Signed-off-by: Marc Branchaud --- diff.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/diff.c b/diff.c index 11eef1c85..da96577ea 100644 --- a/diff.c +++ b/diff.c @@ -290,9 +290,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb) return 0; } - if (git_diff_heuristic_config(var, value, cb) < 0) - return -1; - if (!strcmp(var, "diff.wserrorhighlight")) { int val = parse_ws_error_highlight(value); if (val < 0) @@ -351,6 +348,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb) if (starts_with(var, "submodule.")) return parse_submodule_config_option(var, value); + if (git_diff_heuristic_config(var, value, cb) < 0) + return -1; + return git_default_config(var, value, cb); } -- 2.13.0.rc1.15.gf67d331ad
[PATCHv3 2/4] diff: have the diff-* builtins configure diff before initializing revisions
This matches how the diff Porcelain works. It makes the plumbing commands respect diff's configuration options, such as indentHeuristic, because init_revisions() calls diff_setup() which fills in the diff_options struct. Signed-off-by: Marc Branchaud --- builtin/diff-files.c | 2 +- builtin/diff-index.c | 2 +- builtin/diff-tree.c| 2 +- t/t4061-diff-indent.sh | 66 ++ 4 files changed, 69 insertions(+), 3 deletions(-) diff --git a/builtin/diff-files.c b/builtin/diff-files.c index 15c61fd8d..a572da9d5 100644 --- a/builtin/diff-files.c +++ b/builtin/diff-files.c @@ -20,9 +20,9 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix) int result; unsigned options = 0; + git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ init_revisions(&rev, prefix); gitmodules_config(); - git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ rev.abbrev = 0; precompose_argv(argc, argv); diff --git a/builtin/diff-index.c b/builtin/diff-index.c index 1af373d00..f084826a2 100644 --- a/builtin/diff-index.c +++ b/builtin/diff-index.c @@ -17,9 +17,9 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) int i; int result; + git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ init_revisions(&rev, prefix); gitmodules_config(); - git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ rev.abbrev = 0; precompose_argv(argc, argv); diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c index 326f88b65..36a3a1976 100644 --- a/builtin/diff-tree.c +++ b/builtin/diff-tree.c @@ -105,9 +105,9 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix) struct setup_revision_opt s_r_opt; int read_stdin = 0; + git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ init_revisions(opt, prefix); gitmodules_config(); - git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ opt->abbrev = 0; opt->diff = 1; opt->disable_stdin = 1; diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh index 556450609..13d3dc96a 100755 --- a/t/t4061-diff-indent.sh +++ b/t/t4061-diff-indent.sh @@ -213,4 +213,70 @@ test_expect_success 'blame: --no-indent-heuristic overrides config' ' compare_blame spaces-expect out-blame2 ' +test_expect_success 'diff-tree: nice spaces with --indent-heuristic' ' + git diff-tree --indent-heuristic -p old new -- spaces.txt >out-diff-tree-compacted && + compare_diff spaces-compacted-expect out-diff-tree-compacted +' + +test_expect_success 'diff-tree: nice spaces with diff.indentHeuristic' ' + git -c diff.indentHeuristic=true diff-tree -p old new -- spaces.txt >out-diff-tree-compacted2 && + compare_diff spaces-compacted-expect out-diff-tree-compacted2 +' + +test_expect_success 'diff-tree: --no-indent-heuristic overrides config' ' + git -c diff.indentHeuristic=true diff-tree --no-indent-heuristic -p old new -- spaces.txt >out-diff-tree && + compare_diff spaces-expect out-diff-tree +' + +test_expect_success 'diff-index: nice spaces with --indent-heuristic' ' + git checkout -B diff-index && + git reset --soft HEAD~ && + git diff-index --indent-heuristic -p old -- spaces.txt >out-diff-index-compacted && + compare_diff spaces-compacted-expect out-diff-index-compacted && + git checkout -f master +' + +test_expect_success 'diff-index: nice spaces with diff.indentHeuristic' ' + git checkout -B diff-index && + git reset --soft HEAD~ && + git -c diff.indentHeuristic=true diff-index -p old -- spaces.txt >out-diff-index-compacted2 && + compare_diff spaces-compacted-expect out-diff-index-compacted2 && + git checkout -f master +' + +test_expect_success 'diff-index: --no-indent-heuristic overrides config' ' + git checkout -B diff-index && + git reset --soft HEAD~ && + git -c diff.indentHeuristic=true diff-index --no-indent-heuristic -p old -- spaces.txt >out-diff-index && + compare_diff spaces-expect out-diff-index && + git checkout -f master +' + +test_expect_success 'diff-files: nice spaces with diff.indentHeuristic' ' + git checkout -B diff-files && + git reset HEAD~ && + git -c diff.indentHeuristic=true diff-files -p spaces.txt >out-diff-files-raw && + grep -v index out-diff-files-raw >out-diff-files
[PATCHv2 2/3] diff: have the diff-* builtins configure diff before initializing revisions
This makes the commands respect diff configuration options, such as indentHeuristic, because init_revisions() calls diff_setup() which fills in the diff_options struct. Signed-off-by: Marc Branchaud --- builtin/diff-files.c | 2 +- builtin/diff-index.c | 2 +- builtin/diff-tree.c| 2 +- t/t4061-diff-indent.sh | 66 ++ 4 files changed, 69 insertions(+), 3 deletions(-) diff --git a/builtin/diff-files.c b/builtin/diff-files.c index 15c61fd8d..a572da9d5 100644 --- a/builtin/diff-files.c +++ b/builtin/diff-files.c @@ -20,9 +20,9 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix) int result; unsigned options = 0; + git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ init_revisions(&rev, prefix); gitmodules_config(); - git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ rev.abbrev = 0; precompose_argv(argc, argv); diff --git a/builtin/diff-index.c b/builtin/diff-index.c index 1af373d00..f084826a2 100644 --- a/builtin/diff-index.c +++ b/builtin/diff-index.c @@ -17,9 +17,9 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) int i; int result; + git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ init_revisions(&rev, prefix); gitmodules_config(); - git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ rev.abbrev = 0; precompose_argv(argc, argv); diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c index 326f88b65..36a3a1976 100644 --- a/builtin/diff-tree.c +++ b/builtin/diff-tree.c @@ -105,9 +105,9 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix) struct setup_revision_opt s_r_opt; int read_stdin = 0; + git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ init_revisions(opt, prefix); gitmodules_config(); - git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ opt->abbrev = 0; opt->diff = 1; opt->disable_stdin = 1; diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh index 556450609..13d3dc96a 100755 --- a/t/t4061-diff-indent.sh +++ b/t/t4061-diff-indent.sh @@ -213,4 +213,70 @@ test_expect_success 'blame: --no-indent-heuristic overrides config' ' compare_blame spaces-expect out-blame2 ' +test_expect_success 'diff-tree: nice spaces with --indent-heuristic' ' + git diff-tree --indent-heuristic -p old new -- spaces.txt >out-diff-tree-compacted && + compare_diff spaces-compacted-expect out-diff-tree-compacted +' + +test_expect_success 'diff-tree: nice spaces with diff.indentHeuristic' ' + git -c diff.indentHeuristic=true diff-tree -p old new -- spaces.txt >out-diff-tree-compacted2 && + compare_diff spaces-compacted-expect out-diff-tree-compacted2 +' + +test_expect_success 'diff-tree: --no-indent-heuristic overrides config' ' + git -c diff.indentHeuristic=true diff-tree --no-indent-heuristic -p old new -- spaces.txt >out-diff-tree && + compare_diff spaces-expect out-diff-tree +' + +test_expect_success 'diff-index: nice spaces with --indent-heuristic' ' + git checkout -B diff-index && + git reset --soft HEAD~ && + git diff-index --indent-heuristic -p old -- spaces.txt >out-diff-index-compacted && + compare_diff spaces-compacted-expect out-diff-index-compacted && + git checkout -f master +' + +test_expect_success 'diff-index: nice spaces with diff.indentHeuristic' ' + git checkout -B diff-index && + git reset --soft HEAD~ && + git -c diff.indentHeuristic=true diff-index -p old -- spaces.txt >out-diff-index-compacted2 && + compare_diff spaces-compacted-expect out-diff-index-compacted2 && + git checkout -f master +' + +test_expect_success 'diff-index: --no-indent-heuristic overrides config' ' + git checkout -B diff-index && + git reset --soft HEAD~ && + git -c diff.indentHeuristic=true diff-index --no-indent-heuristic -p old -- spaces.txt >out-diff-index && + compare_diff spaces-expect out-diff-index && + git checkout -f master +' + +test_expect_success 'diff-files: nice spaces with diff.indentHeuristic' ' + git checkout -B diff-files && + git reset HEAD~ && + git -c diff.indentHeuristic=true diff-files -p spaces.txt >out-diff-files-raw && + grep -v index out-diff-files-raw >out-diff-files-compacted && + compare_diff spaces-compacted-exp
[PATCH 3/3] diff: enable indent heuristic by default
From: Stefan Beller The feature was included in v2.11 (released 2016-11-29) and we got no negative feedback. Quite the opposite, all feedback we got was positive. Turn it on by default. Users who dislike the feature can turn it off by setting diff.compactionHeuristic (which includes plumbing commands, see prior patches). Signed-off-by: Stefan Beller Signed-off-by: Marc Branchaud --- diff.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/diff.c b/diff.c index da96577ea..2c47ccb4a 100644 --- a/diff.c +++ b/diff.c @@ -27,7 +27,7 @@ #endif static int diff_detect_rename_default; -static int diff_indent_heuristic; /* experimental */ +static int diff_indent_heuristic = 1; static int diff_rename_limit_default = 400; static int diff_suppress_blank_empty; static int diff_use_color_default = -1; -- 2.13.0.rc1.15.gf67d331ad
[PATCHv2 1/3] diff: make the indent heuristic part of diff's basic configuration
This heuristic was originally introduced as an experimental feature, and therefore part of the UI configuration. But the user often sees diffs generated by plumbing commands like diff-tree. Moving the indent heuristic into diff's basic configuration prepares the way for diff plumbing commands to respect the setting. The heuristic itself merely makes the diffs more aesthetically pleasing, without changing their correctness. Scripts that rely on the diff plumbing commands should not care whether or not the heuristic is employed. Signed-off-by: Marc Branchaud --- diff.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/diff.c b/diff.c index 11eef1c85..da96577ea 100644 --- a/diff.c +++ b/diff.c @@ -290,9 +290,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb) return 0; } - if (git_diff_heuristic_config(var, value, cb) < 0) - return -1; - if (!strcmp(var, "diff.wserrorhighlight")) { int val = parse_ws_error_highlight(value); if (val < 0) @@ -351,6 +348,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb) if (starts_with(var, "submodule.")) return parse_submodule_config_option(var, value); + if (git_diff_heuristic_config(var, value, cb) < 0) + return -1; + return git_default_config(var, value, cb); } -- 2.13.0.rc1.15.gf67d331ad
[PATCHv2 0/3] Make diff plumbing commands respect the indentHeuristic.
v2: Fixed up the commit messages and added tests. Marc Branchaud (2): diff: make the indent heuristic part of diff's basic configuration diff: have the diff-* builtins configure diff before initializing revisions Stefan Beller (1): diff: enable indent heuristic by default builtin/diff-files.c | 2 +- builtin/diff-index.c | 2 +- builtin/diff-tree.c| 2 +- diff.c | 8 +++--- t/t4061-diff-indent.sh | 66 ++ 5 files changed, 73 insertions(+), 7 deletions(-) -- 2.13.0.rc1.15.gf67d331ad
[PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.
So here's my attempt at fixing this. The thing I was missing is that init_revisions() calls diff_setup(), which sets the xdl options. It's therefore necessary to have the diff_indent_heuristic flag set before calling init_revisions(). A naive way to get the indentHeuristic config option respected in the diff-* plumbing commands is to make them use the git_diff_heuristic_config() callback right at the start of their main cmd functions. But I did not like that for two reasons: * It would make these commands invoke git_config() twice. * It doesn't avoid the problem if/when someone creates a new diff-something plumbing command, and forgets to set the diff_indent_heuristic flag before calling init_revisions(). So instead I chose to make the indentHeuristic option part of diff's basic configuration, and in each of the diff plumbing commands I moved the call to git_config() before the call to init_revisions(). This still doesn't really future-proof things for possible new diff plumbing commands, because someone could still invoke init_revisions() before setting up diff's basic configuration. But I don't see an obvious way of ensuring that the diff_indent_heuristic flag is respected regardless of when diff_setup() is invoked. M. Marc Branchaud (2): Make the indent heuristic part of diff's basic configuration. Have the diff-* builtins configure diff before initializing revisions. builtin/diff-files.c | 2 +- builtin/diff-index.c | 2 +- builtin/diff-tree.c | 2 +- diff.c | 6 +++--- 4 files changed, 6 insertions(+), 6 deletions(-)
[PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.
This makes the commands respect diff configuration options, such as indentHeuristic. Signed-off-by: Marc Branchaud --- builtin/diff-files.c | 2 +- builtin/diff-index.c | 2 +- builtin/diff-tree.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/diff-files.c b/builtin/diff-files.c index 15c61fd8d..a572da9d5 100644 --- a/builtin/diff-files.c +++ b/builtin/diff-files.c @@ -20,9 +20,9 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix) int result; unsigned options = 0; + git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ init_revisions(&rev, prefix); gitmodules_config(); - git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ rev.abbrev = 0; precompose_argv(argc, argv); diff --git a/builtin/diff-index.c b/builtin/diff-index.c index 1af373d00..f084826a2 100644 --- a/builtin/diff-index.c +++ b/builtin/diff-index.c @@ -17,9 +17,9 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) int i; int result; + git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ init_revisions(&rev, prefix); gitmodules_config(); - git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ rev.abbrev = 0; precompose_argv(argc, argv); diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c index 326f88b65..36a3a1976 100644 --- a/builtin/diff-tree.c +++ b/builtin/diff-tree.c @@ -105,9 +105,9 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix) struct setup_revision_opt s_r_opt; int read_stdin = 0; + git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ init_revisions(opt, prefix); gitmodules_config(); - git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ opt->abbrev = 0; opt->diff = 1; opt->disable_stdin = 1; -- 2.13.0.rc1.15.g7dbea34e1.dirty
[PATCH 1/2] Make the indent heuristic part of diff's basic configuration.
Signed-off-by: Marc Branchaud --- diff.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/diff.c b/diff.c index 11eef1c85..da96577ea 100644 --- a/diff.c +++ b/diff.c @@ -290,9 +290,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb) return 0; } - if (git_diff_heuristic_config(var, value, cb) < 0) - return -1; - if (!strcmp(var, "diff.wserrorhighlight")) { int val = parse_ws_error_highlight(value); if (val < 0) @@ -351,6 +348,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb) if (starts_with(var, "submodule.")) return parse_submodule_config_option(var, value); + if (git_diff_heuristic_config(var, value, cb) < 0) + return -1; + return git_default_config(var, value, cb); } -- 2.13.0.rc1.15.g7dbea34e1.dirty
BUG: diff-{index,files,tree} (and git-gui) do not respect the diff.indentHeuristic config setting
So I have diff.indentHeuristic = true and I noticed that git-gui was not using the heuristic. This is because git-gui uses diff-index, and that does not respect the config setting, even though it supports the --indent-heuristic option. And it looks like diff-files and diff-tree also have the same problem. I tried a couple of quick-n-dirty things to fix it in diff-index, without success, and I've run out of git-hacking tame, so all I can do for now is throw out a bug report. diff-index.c explicitly says "no 'diff' UI options" since 83ad63cfeb ("diff: do not use configuration magic at the core-level", 2006-07-08), so maybe this needs to be fixed in git-gui (and maybe elsewhere), but to me it feels like the diff-foo commands should respect the setting. (CC'ing Michael Haggerty, who added the heuristic.) (This is git v2.12.2.) M.
Re: [PATCH] Do not require Python for the git-remote-{bzr,hg} placeholder scripts
On 2017-03-03 05:57 AM, Sebastian Schuberth wrote: It does not make sense for these placeholder scripts to depend on Python just because the real scripts do. At the example of Git for Windows, we would not even be able to see those warnings as it does not ship with Python. So just use plain shell scripts instead. Just a niggle: This change moves the warning message from stderr to stdout. M. Signed-off-by: Sebastian Schuberth --- contrib/remote-helpers/git-remote-bzr | 16 +++- contrib/remote-helpers/git-remote-hg | 16 +++- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr index 712a137..ccc4aea 100755 --- a/contrib/remote-helpers/git-remote-bzr +++ b/contrib/remote-helpers/git-remote-bzr @@ -1,13 +1,11 @@ -#!/usr/bin/env python +#!/bin/sh -import sys - -sys.stderr.write('WARNING: git-remote-bzr is now maintained independently.\n') -sys.stderr.write('WARNING: For more information visit https://github.com/felipec/git-remote-bzr\n') - -sys.stderr.write('''WARNING: +cat <<'EOT' +WARNING: git-remote-bzr is now maintained independently. +WARNING: For more information visit https://github.com/felipec/git-remote-bzr +WARNING: WARNING: You can pick a directory on your $PATH and download it, e.g.: -WARNING: $ wget -O $HOME/bin/git-remote-bzr \\ +WARNING: $ wget -O $HOME/bin/git-remote-bzr \ WARNING: https://raw.github.com/felipec/git-remote-bzr/master/git-remote-bzr WARNING: $ chmod +x $HOME/bin/git-remote-bzr -''') +EOT diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg index 4255ad6..dfda44f 100755 --- a/contrib/remote-helpers/git-remote-hg +++ b/contrib/remote-helpers/git-remote-hg @@ -1,13 +1,11 @@ -#!/usr/bin/env python +#!/bin/sh -import sys - -sys.stderr.write('WARNING: git-remote-hg is now maintained independently.\n') -sys.stderr.write('WARNING: For more information visit https://github.com/felipec/git-remote-hg\n') - -sys.stderr.write('''WARNING: +cat <<'EOT' +WARNING: git-remote-hg is now maintained independently. +WARNING: For more information visit https://github.com/felipec/git-remote-hg +WARNING: WARNING: You can pick a directory on your $PATH and download it, e.g.: -WARNING: $ wget -O $HOME/bin/git-remote-hg \\ +WARNING: $ wget -O $HOME/bin/git-remote-hg \ WARNING: https://raw.github.com/felipec/git-remote-hg/master/git-remote-hg WARNING: $ chmod +x $HOME/bin/git-remote-hg -''') +EOT -- https://github.com/git/git/pull/333
Re: [RFC][Git GUI] Make Commit message field in git GUI re sizable.
On 2017-02-22 06:59 AM, Jessie Hernandez wrote: HI, the reason why it is fixed, is because commit messages should be wrapped at 76 characters to be used in mails. So it helps you with the wrapping. Bert Right ok. I understand. Knowing this I think I might start writing my commit messages differently then. You can configure gui.commitMsgWidth if you don't like the default (which is 75, not 76). M. Thank you for this. Regards - Jessie Hernandez On Wed, Feb 22, 2017 at 10:27 AM, Jessie Hernandez wrote: Hi all, I have been using git for a few years now and really like the software. I have a small annoyance and was wondering if I could get the communities view on this. When using git GUI I find it handy to be able to re-size the "Unstaged Changes" and the "Staged Changed" fields. I would like the same thing for the "Commit Message" field, or to have it re-size with the git GUI window. I can re-size the "Commit Message" vertically when making the "Modified" panel smaller. Does this make sense? I would be happy to get into more detail if that is necessary or if something is not clear. Thank you. - Jessie Hernandez
Re: [PATCH 00/13] gitk: tweak rendering of remote-tracking references
On 2016-12-19 11:44 AM, Michael Haggerty wrote: This patch series changes a bunch of details about how remote-tracking references are rendered in the commit list of gitk: I don't see this series in git v2.12.0-rc0, nor in Paul's gitk repo. I hope this is an oversight, and not that the series got dropped for some reason. M. * Omit the "remote/" prefix on normal remote-tracking references. They are already distinguished via their two-tone rendering and (usually) longer names, and this change saves a lot of visual clutter and horizontal space. * Render remote-tracking references that have more than the usual three slashes like origin/foo/bar ^^^ rather than origin/foo/bar (formerly remotes/origin/foo/bar) ^^^ ^^^ , where the indicated part is the prefix that is rendered in a different color. Usually, such a reference represents a remote branch that contains a slash in its name, so the new split more accurately portrays the separation between remote name and remote branch name. * Introduce a separate constant to specify the background color used for the branch name part of remote-tracking references, to allow it to differ from the color used for local branches (which by default is bright green). * Change the default background colors for remote-tracking branches to light brown and brown (formerly they were pale orange and bright green). I understand that the colors of pixels on computer screens is an even more emotional topic that that of bikesheds, so I implemented the last change as a separate commit, the last one in the series. Feel free to drop it if you don't want the default color change. Along the way, I did a bunch of refactoring in the area to make these changes easier, and introduced a constant for the background color of "other" references so that it can also be adjusted by users. (Unfortunately, these colors can only be adjusted by editing the configuration file. Someday it would be nice to allow them to be configured via the preferences dialog.) It's been a while since I've written any Tcl code, so I apologize in advance for any howlers :-) This branch applies against the `master` branch in git://ozlabs.org/~paulus/gitk. Michael Michael Haggerty (13): gitk: when processing tag labels, don't use `marks` as scratch space gitk: keep track of tag types in a separate `types` array gitk: use a type "tags" to indicate abbreviated tags gitk: use a type "mainhead" to indicate the main HEAD branch gitk: fill in `wvals` as the tags are first processed gitk: simplify regexp gitk: extract a method `remotereftext` gitk: only change the color of the "remote" part of remote refs gitk: shorten labels displayed for modern remote-tracking refs gitk: use type "remote" for remote-tracking references gitk: introduce a constant otherrefbgcolor gitk: add a configuration setting `remoterefbgcolor` gitk: change the default colors for remote-tracking references gitk | 114 --- 1 file changed, 76 insertions(+), 38 deletions(-)
Re: [RFC] stash --continue
On 2017-01-19 04:30 PM, Johannes Schindelin wrote: At this point I will stop commenting on this issue, as I have said all that I wanted to say about it, at least once. If I failed to get my points across so far, I simply won't be understood. Yes, we're obviously looking at this from completely different perspectives. Stephan (or whoever) if you decide to do this work, I will be content with whichever way you choose to go. M.
Re: [RFC] stash --continue
On 2017-01-19 10:49 AM, Johannes Schindelin wrote: Hi Marc, On Wed, 18 Jan 2017, Marc Branchaud wrote: On 2017-01-18 11:34 AM, Johannes Schindelin wrote: On Wed, 18 Jan 2017, Marc Branchaud wrote: On 2017-01-16 05:54 AM, Johannes Schindelin wrote: On Mon, 16 Jan 2017, Stephan Beyer wrote: a git-newbie-ish co-worker uses git-stash sometimes. Last time he used "git stash pop", he got into a merge conflict. After he resolved the conflict, he did not know what to do to get the repository into the wanted state. In his case, it was only "git add " followed by a "git reset" and a "git stash drop", but there may be more involved cases when your index is not clean before "git stash pop" and you want to have your index as before. This led to the idea to have something like "git stash --continue"[1] More like "git stash pop --continue". Without the "pop" command, it does not make too much sense. Why not? git should be able to remember what stash command created the conflict. Why should I have to? Maybe the fire alarm goes off right when I run the stash command, and by the time I get back to it I can't remember which operation I did. It would be nice to be able to tell git to "just finish off (or abort) the stash operation, whatever it was". That reeks of a big potential for confusion. Imagine for example a total Git noob who calls `git stash list`, scrolls two pages down, then hits `q` by mistake. How would you explain to that user that `git stash --continue` does not continue showing the list at the third page? Sorry, but I have trouble taking that example seriously. It assumes such a level of "noobness" that the user doesn't even understand how standard command output paging works, not just with git but with any shell command. Yeah, well, I thought you understood what I meant. The example was the best I could come up with quickly, and it only tried to show that there are *other* stash operations that one might perceive to happen at the same time as the "pop" operation, so your flimsical comment "why not continue the latest operation" may very well be ambiguous. And if it is not ambiguous in "stash", it certainly will be in other Git operations. And therefore, having a DWIM in "stash" to allow "--continue" without any specific subcommand, but not having it in other Git commands, is just a very poor user interface design. It is prone to confuse users, which is always a hallmark of a bad user interface. Please don't underestimate the power of syntactic consistency in helping users achieve their goals. Having some commands use "git foo --continue" while others use "git foo bar --continue" *will* confuse people, regardless of how logical the reasons for those differences. But in the case of stash, I still don't see the utility in having operation-specific continuation. Consider the following sequence (as you say, this doesn't work yet, but making it work seems reasonable): git stash pop # creates some conflicts git stash apply stash@{4} # creates some other conflicts # (User resolves the conflicts created by the pop.) git stash pop --continue Given the description of the original proposal (do "git reset; git stash drop"), what's the state of the index and the working tree? In particular, what has the user gained by continuing just that pop? Another thing to ask is, how common is such a scenario likely to be? I suggest that it will be far more common for users to resolve all the conflicts and then want to continue all their interrupted stash operations. If so, fussily forcing them to explicitly continue the pop and the apply is just a waste. Hence my objection to "git stash --continue". No argument in favor of "git stash --continue" I heard so far comes even close to being convincing. Well, what about the potential for a slippery slope? If the user is forced to be specific about continuing either a pop or an apply, why wouldn't git allow them to be specific about *which* pop or apply they want to continue? Consider another hypothetical scenario: git stash pop # creates some conflicts git stash pop # creates some more conflicts git stash pop # creates even more conflicts # (User resolves the conflicts created by second pop.) git stash pop --continue # Oops, there's still some unresovled pops! Obviously the user isn't ready to finish off all the pops, so they'll want some way to specify which pop to continue. Dealing with that just feels like a lot of work for minimal benefit. Even worse: `git stash` (without arguments) defaults to the `save` operation, so any user who does not read the documentation (and who does?) woul
Re: [RFC] stash --continue
On 2017-01-18 11:34 AM, Johannes Schindelin wrote: Hi Marc, On Wed, 18 Jan 2017, Marc Branchaud wrote: On 2017-01-16 05:54 AM, Johannes Schindelin wrote: On Mon, 16 Jan 2017, Stephan Beyer wrote: a git-newbie-ish co-worker uses git-stash sometimes. Last time he used "git stash pop", he got into a merge conflict. After he resolved the conflict, he did not know what to do to get the repository into the wanted state. In his case, it was only "git add " followed by a "git reset" and a "git stash drop", but there may be more involved cases when your index is not clean before "git stash pop" and you want to have your index as before. This led to the idea to have something like "git stash --continue"[1] More like "git stash pop --continue". Without the "pop" command, it does not make too much sense. Why not? git should be able to remember what stash command created the conflict. Why should I have to? Maybe the fire alarm goes off right when I run the stash command, and by the time I get back to it I can't remember which operation I did. It would be nice to be able to tell git to "just finish off (or abort) the stash operation, whatever it was". That reeks of a big potential for confusion. Imagine for example a total Git noob who calls `git stash list`, scrolls two pages down, then hits `q` by mistake. How would you explain to that user that `git stash --continue` does not continue showing the list at the third page? Sorry, but I have trouble taking that example seriously. It assumes such a level of "noobness" that the user doesn't even understand how standard command output paging works, not just with git but with any shell command. Even worse: `git stash` (without arguments) defaults to the `save` operation, so any user who does not read the documentation (and who does?) would assume that `git stash --continue` *also* implies `save`. Like the first example, your user is trying to "continue" a command that is already complete. It's like try to do "git rebase --continue" when there's no rebase operation underway. Now, maybe there is some way for "git stash save" (implied or explicit) to stop partway through the operation. I can't imagine such a situation (out of disk space, maybe?), particularly where the user would expect "git stash save" to leave things in a half-finished state. To me "git stash save" should be essentially all-or-nothing. However, if there were such a partial-failure scenario, then I think it would be perfectly reasonable for "git stash --continue" to finish the save operation, assuming that the failure condition has been resolved. If that was not enough, there would still be the overall design of Git's user interface. You can call it confusing, inconsistent, with a lot of room for improvement, and you would be correct. But none of Git's commands has a `--continue` option that remembers the latest subcommand and continues that. To introduce that behavior in `git stash` would disimprove the situation. I think it's more the case that none of the current continuable commands have subcommands (though I can't think of all the continuable or abortable operations offhand, so maybe I'm wrong). I think we're discussing new UI ground here. And since the pattern is already "git foo --continue", it seems more consistent to me for it to be "git stash --continue" as well. Especially since there can be only one partially-complete stash sub-operation at one time (per workdir, at least). So there's no reason to change the pattern just for the stash command. Think of it this way: All the currently continuable/abortable commands put the repository in a shaky state, where performing certain other operations would be ill advised. Attempting to start a rebase while a merge conflict is unresolved, for example. IIRC, git actually tries to stop users from shooting their feet in this way. And so it should be for the stash operation: If applying a stash yields a conflict, it has to be resolved or aborted before something like a rebase or merge is attempted. It doesn't matter which stash subcommand created the shaky situation. In the long run, I think there's even the possibility of generic "git continue" and "git abort" commands, that simply continue or abort the current partially-complete operation, whatever it is. (Isn't that the ultimate goal of all the "sequencer" work? I admit I have not been following that effort.) With every new feature, it is not enough to consider its benefits. You always have to take the potential fallout into account, too. Agreed. At least `git stash pop --continue` would be consistent with all other `--continue` options in Git that I can think of... Alas, I disagree! M.
Re: [RFC] stash --continue
On 2017-01-16 05:54 AM, Johannes Schindelin wrote: Hi Stephan, On Mon, 16 Jan 2017, Stephan Beyer wrote: a git-newbie-ish co-worker uses git-stash sometimes. Last time he used "git stash pop", he got into a merge conflict. After he resolved the conflict, he did not know what to do to get the repository into the wanted state. In his case, it was only "git add " followed by a "git reset" and a "git stash drop", but there may be more involved cases when your index is not clean before "git stash pop" and you want to have your index as before. This led to the idea to have something like "git stash --continue"[1] More like "git stash pop --continue". Without the "pop" command, it does not make too much sense. Why not? git should be able to remember what stash command created the conflict. Why should I have to? Maybe the fire alarm goes off right when I run the stash command, and by the time I get back to it I can't remember which operation I did. It would be nice to be able to tell git to "just finish off (or abort) the stash operation, whatever it was". M.
Re: [PATCH 00/13] gitk: tweak rendering of remote-tracking references
On 2016-12-22 03:15 AM, Michael Haggerty wrote: On 12/21/2016 08:07 PM, Marc Branchaud wrote: On 2016-12-20 07:05 PM, Michael Haggerty wrote: On 12/20/2016 04:01 PM, Marc Branchaud wrote: [...] Please don't change the remotebgcolor default. Also, perhaps the default remoterefbgcolor should be set remoterefbgcolor $headbgcolor ? I say this because when I applied the series, without the last patch, I was miffed that the remote/ref colour had changed. This is a one-time inconvenience that gitk developers will experience. I doubt that users jump backwards and forwards in gitk versions very often. In what way do you mean it's restricted to gitk developers? Maybe I misunderstood your objection. While developing this, I realized that the very first time your run gitk (i.e., when you don't already have a configuration file), it writes the then-current default colors into your configuration file. If you later switch gitk versions to a version with different default colors, the colors from the first-run version are preserved (unless the names of the variables used to hold the colors are changed). So if you would run the tip version of my branch first, then the parent of that version, you would continue to see the colors as modified by the final commit. If you then switch to the master version, the remote branch names go back to green (because it goes back to using `headbgcolor` again), but the remote prefix stays light brown. I thought you were unhappy about some form of this unexpected persistence problem. But this problem will mostly be experienced by gitk developers who jump back and forth between versions. A normal user probably mostly moves forward through version numbers, and only occasionally. Such a user, jumping from master to the tip of my branch (assuming they haven't customized anything), would see * local branches stay lime * remote branch prefixes stay pale orange (they don't change to light brown because the pale orange color from master is stored in their configuration file) * remote branch names change from lime to brown (because the `remoterefbgcolor` configuration setting didn't exist before) Patch 12 introduces remoterefbgcolor, with a specific default value. Prior to that, the "ref part" of remote refs was rendered with headbgcolor. Users who changed their headbgcolor are used to seeing the "ref part" of remote refs in the same color as their local heads. Applying patch 12 changes the "ref part" color of remote refs, for such users. All I'm saying is that make the remoterefbgcolor default be $headbgcolor avoids this. For somebody who thinks that most people will want local and remote-tracking branch names to be rendered in the same color, your suggestion would be an improvement. But for somebody like me who thinks it is a good idea to render remote-tracking branch names in a different color than local branch names, this is a feature, not a bug. Even a user who explicitly configured `headbgcolor` to, say, purple, wasn't necessarily expressing a wish to have remote-tracking branch names rendered in purple. Until now they had no choice to set these colors separately! So even for somebody who configured this setting before, I think that having the remote-tracking branch names change color when the user upgrades to this version is a good thing, because it lets the user know that these two things can now be colored independently. If they don't like the new default brown color, such a user knows where to change it (even to make it agree with `headbgcolor` if they want that). But I understand that this is a matter of personal preference. I have but one "vote" :-) I think we understand each other, and that we disagree. I don't feel strongly about it though, so if you want to go this way that's fine by me. Your case would be helped if the various ref colours were exposed in the preferences dialog. Someone who upgrades to your gitk and doesn't like the default remoterefbgcolor has to track down the right setting, close all running gitk instances, and hand-edit ~/.gitk. But I think improving gitk's colour-preferences dialog is a bit beyond the scope of your topic... M.
Re: [PATCH 00/13] gitk: tweak rendering of remote-tracking references
On 2016-12-20 07:05 PM, Michael Haggerty wrote: On 12/20/2016 04:01 PM, Marc Branchaud wrote: On 2016-12-19 11:44 AM, Michael Haggerty wrote: This patch series changes a bunch of details about how remote-tracking references are rendered in the commit list of gitk: Thanks for this! I like the new, compact look very much! That said, I remember when I was a new git user and I leaned heavily on gitk to understand how references worked. It was particularly illuminating to see the remote references distinctly labeled, and the fact that they were "remotes/origin/foo" gave me an Aha! moment where I came to understand that the refs hierarchy is more flexible than just the conventions coded into git itself. I eventually felt free to create my own, private ref hierarchies. I am in no way opposed to this series. I just wanted to point out that there was some utility in those labels. It makes me think that it might be worthwhile for gitk to have a "raw-refs" mode, that shows the full "refs/foo/bar/baz" paths of all the heads, tags, and whatever else. It could be a useful teaching tool for git. Yes, I understand that the longer names might be useful for beginners, and the full names even more so. However, I think once a user has that "aha!" moment, the space wasted on all the redundant words is a real impediment to gitk's usability. It is common to have a few references on a single commit (especially if you pull from multiple remotes), in which case the summary line is completely invisible (and therefore its context menu is unreachable). I don't like the idea of dumbing down the UI permanently based on what users need at the very beginning of their Git usage. I agree. Would it be possible to use the short names in the UI but to add the full names to a tooltip or to the context menu? I don't know -- my Tk-fu is weak. * Omit the "remote/" prefix on normal remote-tracking references. They If you re-roll, s:remote/:remotes/:. Thanks. are already distinguished via their two-tone rendering and (usually) longer names, and this change saves a lot of visual clutter and horizontal space. * Render remote-tracking references that have more than the usual three slashes like origin/foo/bar ^^^ rather than origin/foo/bar (formerly remotes/origin/foo/bar) ^^^ ^^^ , where the indicated part is the prefix that is rendered in a different color. Usually, such a reference represents a remote branch that contains a slash in its name, so the new split more accurately portrays the separation between remote name and remote branch name. *Love* this change! :) * Introduce a separate constant to specify the background color used for the branch name part of remote-tracking references, to allow it to differ from the color used for local branches (which by default is bright green). * Change the default background colors for remote-tracking branches to light brown and brown (formerly they were pale orange and bright green). Please don't change the remotebgcolor default. Also, perhaps the default remoterefbgcolor should be set remoterefbgcolor $headbgcolor ? I say this because when I applied the series, without the last patch, I was miffed that the remote/ref colour had changed. This is a one-time inconvenience that gitk developers will experience. I doubt that users jump backwards and forwards in gitk versions very often. In what way do you mean it's restricted to gitk developers? Patch 12 introduces remoterefbgcolor, with a specific default value. Prior to that, the "ref part" of remote refs was rendered with headbgcolor. Users who changed their headbgcolor are used to seeing the "ref part" of remote refs in the same color as their local heads. Applying patch 12 changes the "ref part" color of remote refs, for such users. All I'm saying is that make the remoterefbgcolor default be $headbgcolor avoids this. But, honestly, I don't feel strongly about it. M.
Re: [PATCH 00/13] gitk: tweak rendering of remote-tracking references
On 2016-12-20 05:17 PM, Paul Mackerras wrote: On Tue, Dec 20, 2016 at 10:01:15AM -0500, Marc Branchaud wrote: On 2016-12-19 11:44 AM, Michael Haggerty wrote: This patch series changes a bunch of details about how remote-tracking references are rendered in the commit list of gitk: Thanks for this! I like the new, compact look very much! That said, I remember when I was a new git user and I leaned heavily on gitk to understand how references worked. It was particularly illuminating to see the remote references distinctly labeled, and the fact that they were "remotes/origin/foo" gave me an Aha! moment where I came to understand that the refs hierarchy is more flexible than just the conventions coded into git itself. I eventually felt free to create my own, private ref hierarchies. I am in no way opposed to this series. I just wanted to point out that there was some utility in those labels. It makes me think that it might be worthwhile for gitk to have a "raw-refs" mode, that shows the full "refs/foo/bar/baz" paths of all the heads, tags, and whatever else. It could be a useful teaching tool for git. Do you think we should have a checkbox in the preferences dialog to select whether to display the long form or the short form? Mmmm, more knobs! No, I don't think that's necessary. Such a setting would probably just confuse people. Plus it's not something anyone is likely to want to change anyway. Maybe if there were an "Advanced" tab in the settings dialog, but even then it seems like overkill. I suspect there are better ways to teach people about the refs hierarchy than cluttering up gitk. These may even already exist -- I've been a git user for 8 years now, so I'm sure my perspective of the learning curve is skewed. M.
Re: [PATCH 00/13] gitk: tweak rendering of remote-tracking references
On 2016-12-19 11:44 AM, Michael Haggerty wrote: This patch series changes a bunch of details about how remote-tracking references are rendered in the commit list of gitk: Thanks for this! I like the new, compact look very much! That said, I remember when I was a new git user and I leaned heavily on gitk to understand how references worked. It was particularly illuminating to see the remote references distinctly labeled, and the fact that they were "remotes/origin/foo" gave me an Aha! moment where I came to understand that the refs hierarchy is more flexible than just the conventions coded into git itself. I eventually felt free to create my own, private ref hierarchies. I am in no way opposed to this series. I just wanted to point out that there was some utility in those labels. It makes me think that it might be worthwhile for gitk to have a "raw-refs" mode, that shows the full "refs/foo/bar/baz" paths of all the heads, tags, and whatever else. It could be a useful teaching tool for git. * Omit the "remote/" prefix on normal remote-tracking references. They If you re-roll, s:remote/:remotes/:. are already distinguished via their two-tone rendering and (usually) longer names, and this change saves a lot of visual clutter and horizontal space. * Render remote-tracking references that have more than the usual three slashes like origin/foo/bar ^^^ rather than origin/foo/bar (formerly remotes/origin/foo/bar) ^^^ ^^^ , where the indicated part is the prefix that is rendered in a different color. Usually, such a reference represents a remote branch that contains a slash in its name, so the new split more accurately portrays the separation between remote name and remote branch name. *Love* this change! :) * Introduce a separate constant to specify the background color used for the branch name part of remote-tracking references, to allow it to differ from the color used for local branches (which by default is bright green). * Change the default background colors for remote-tracking branches to light brown and brown (formerly they were pale orange and bright green). Please don't change the remotebgcolor default. Also, perhaps the default remoterefbgcolor should be set remoterefbgcolor $headbgcolor ? I say this because when I applied the series, without the last patch, I was miffed that the remote/ref colour had changed. Plus I think there's utility in having local and remote branch names in the same colour, again especially for new users. It helps emphasize their similarities, and demystify some of git's internal magic. M. I understand that the colors of pixels on computer screens is an even more emotional topic that that of bikesheds, so I implemented the last change as a separate commit, the last one in the series. Feel free to drop it if you don't want the default color change. Along the way, I did a bunch of refactoring in the area to make these changes easier, and introduced a constant for the background color of "other" references so that it can also be adjusted by users. (Unfortunately, these colors can only be adjusted by editing the configuration file. Someday it would be nice to allow them to be configured via the preferences dialog.) It's been a while since I've written any Tcl code, so I apologize in advance for any howlers :-) This branch applies against the `master` branch in git://ozlabs.org/~paulus/gitk. Michael Michael Haggerty (13): gitk: when processing tag labels, don't use `marks` as scratch space gitk: keep track of tag types in a separate `types` array gitk: use a type "tags" to indicate abbreviated tags gitk: use a type "mainhead" to indicate the main HEAD branch gitk: fill in `wvals` as the tags are first processed gitk: simplify regexp gitk: extract a method `remotereftext` gitk: only change the color of the "remote" part of remote refs gitk: shorten labels displayed for modern remote-tracking refs gitk: use type "remote" for remote-tracking references gitk: introduce a constant otherrefbgcolor gitk: add a configuration setting `remoterefbgcolor` gitk: change the default colors for remote-tracking references gitk | 114 --- 1 file changed, 76 insertions(+), 38 deletions(-)
[PATCHv2] Tweak help auto-correct phrasing.
When auto-correct is enabled, an invalid git command prints a warning and a continuation message, which differs depending on whether or not help.autoCorrect is positive or negative. With help.autoCorrect = 15: WARNING: You called a Git command named 'lgo', which does not exist. Continuing under the assumption that you meant 'log' in 1.5 seconds automatically... With help.autoCorrect < 0: WARNING: You called a Git command named 'lgo', which does not exist. Continuing under the assumption that you meant 'log' The continuation message's phrasing is awkward. This commit cleans it up. As a bonus, we now use full-sentence strings which make translation easier. With help.autoCorrect = 15: WARNING: You called a Git command named 'lgo', which does not exist. Continuing in 1.5 seconds, assuming that you meant 'log'. With help.autoCorrect < 0: WARNING: You called a Git command named 'lgo', which does not exist. Continuing under the assumption that you meant 'log'. Signed-off-by: Marc Branchaud --- Writing the commit message was more work than the commit! :) M. help.c | 18 -- 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/help.c b/help.c index 53e2a67e00..fc56aa2d76 100644 --- a/help.c +++ b/help.c @@ -381,12 +381,18 @@ const char *help_unknown_cmd(const char *cmd) clean_cmdnames(&main_cmds); fprintf_ln(stderr, _("WARNING: You called a Git command named '%s', " -"which does not exist.\n" -"Continuing under the assumption that you meant '%s'"), - cmd, assumed); - if (autocorrect > 0) { - fprintf_ln(stderr, _("in %0.1f seconds automatically..."), - (float)autocorrect/10.0); +"which does not exist."), + cmd); + if (autocorrect < 0) + fprintf_ln(stderr, + _("Continuing under the assumption that " +"you meant '%s'."), + assumed); + else { + fprintf_ln(stderr, + _("Continuing in %0.1f seconds, " +"assuming that you meant '%s'."), + (float)autocorrect/10.0, assumed); sleep_millisec(autocorrect * 100); } return assumed; -- 2.11.0.1.g75fa99b
[PATCH] Tweak help auto-correct phrasing.
Signed-off-by: Marc Branchaud --- On 2016-12-18 07:48 PM, Chris Packham wrote: > > This feature already exists (although it's not interactive). See > help.autoCorrect in the git-config man page. "git config > help.autoCorrect -1" should to the trick. Awesome, I was unaware of this feature. Thanks! I found the message it prints a bit awkward, so here's a patch to fix it up. Instead of: WARNING: You called a Git command named 'lgo', which does not exist. Continuing under the assumption that you meant 'log' in 1.5 seconds automatically... it's now: WARNING: You called a Git command named 'lgo', which does not exist. Continuing in 1.5 seconds under the assumption that you meant 'log'. M. help.c | 18 -- 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/help.c b/help.c index 53e2a67e00..55350c0673 100644 --- a/help.c +++ b/help.c @@ -381,12 +381,18 @@ const char *help_unknown_cmd(const char *cmd) clean_cmdnames(&main_cmds); fprintf_ln(stderr, _("WARNING: You called a Git command named '%s', " -"which does not exist.\n" -"Continuing under the assumption that you meant '%s'"), - cmd, assumed); - if (autocorrect > 0) { - fprintf_ln(stderr, _("in %0.1f seconds automatically..."), - (float)autocorrect/10.0); +"which does not exist."), + cmd); + if (autocorrect < 0) + fprintf_ln(stderr, + _("Continuing under the assumption that " +"you meant '%s'."), + assumed); + else { + fprintf_ln(stderr, + _("Continuing in %0.1f seconds under the " +"assumption that you meant '%s'."), + (float)autocorrect/10.0, assumed); sleep_millisec(autocorrect * 100); } return assumed; -- 2.11.0.dirty
[PATCH] Release note spelling and phrasing fixups.
Signed-off-by: Marc Branchaud --- Mostly just missing words and what I feel are clarifications. The biggest change is to the "git add -N" item. Not 100% sure I got it right. M. Documentation/RelNotes/2.11.0.txt | 145 +++--- 1 file changed, 72 insertions(+), 73 deletions(-) diff --git a/Documentation/RelNotes/2.11.0.txt b/Documentation/RelNotes/2.11.0.txt index cea2a50..de5892e 100644 --- a/Documentation/RelNotes/2.11.0.txt +++ b/Documentation/RelNotes/2.11.0.txt @@ -57,39 +57,40 @@ UI, Workflows & Features * Even though "git hash-objects", which is a tool to take an on-filesystem data stream and put it into the Git object store, - allowed to perform the "outside-world-to-Git" conversions (e.g. + can perform "outside-world-to-Git" conversions (e.g. end-of-line conversions and application of the clean-filter), and - it had the feature on by default from very early days, its reverse + it has had this feature on by default from very early days, its reverse operation "git cat-file", which takes an object from the Git object - store and externalize for the consumption by the outside world, + store and externalizes it for consumption by the outside world, lacked an equivalent mechanism to run the "Git-to-outside-world" conversion. The command learned the "--filters" option to do so. - * Output from "git diff" can be made easier to read by selecting + * Output from "git diff" can be made easier to read by intelligently selecting which lines are common and which lines are added/deleted - intelligently when the lines before and after the changed section - are the same. A command line option is added to help with the - experiment to find a good heuristics. + when the lines before and after the changed section + are the same. A command line option (--indent-heuristic) and a + configuration variable (diff.indentHeuristic) are added to help with the + experiment to find good heuristics. * In some projects, it is common to use "[RFC PATCH]" as the subject prefix for a patch meant for discussion rather than application. A - new option "--rfc" is a short-hand for "--subject-prefix=RFC PATCH" + new format-patch option "--rfc" is a short-hand for "--subject-prefix=RFC PATCH" to help the participants of such projects. - * "git add --chmod=+x " added recently only toggled the + * "git add --chmod={+,-}x " only changed the executable bit for paths that are either new or modified. This has - been corrected to flip the executable bit for all paths that match + been corrected to change the executable bit for all paths that match the given pathspec. * When "git format-patch --stdout" output is placed as an in-body - header and it uses the RFC2822 header folding, "git am" failed to + header and it uses RFC2822 header folding, "git am" fails to put the header line back into a single logical line. The underlying "git mailinfo" was taught to handle this properly. * "gitweb" can spawn "highlight" to show blob contents with (programming) language-specific syntax highlighting, but only when the language is known. "highlight" can however be told - to make the guess itself by giving it "--force" option, which + to guess the language itself by giving it "--force" option, which has been enabled. * "git gui" l10n to Portuguese. @@ -109,19 +110,19 @@ UI, Workflows & Features history leading to nth parent was looking the other way. * In recent versions of cURL, GSSAPI credential delegation is - disabled by default due to CVE-2011-2192; introduce a configuration - to selectively allow enabling this. + disabled by default due to CVE-2011-2192; introduce a http.delegation + configuration variable to selectively allow enabling this. (merge 26a7b23429 ps/http-gssapi-cred-delegation later to maint). * "git mergetool" learned to honor "-O" to control the order of paths to present to the end user. * "git diff/log --ws-error-highlight=" lacked the corresponding - configuration variable to set it by default. + configuration variable (diff.wsErrorHighlight) to set it by default. - * "git ls-files" learned "--recurse-submodules" option that can be - used to get a listing of tracked files across submodules (i.e. this - only works with "--cached" option, not for listing untracked or + * "git ls-files" learned the "--recurse-submodules" option + to get a listing of tracked files across submodules (i.e. this + only works with the "--cached" option, not for listing untracke
Re: [ANNOUNCE] Git v2.11.0-rc3
On 2016-11-23 06:21 PM, Junio C Hamano wrote: * The original command line syntax for "git merge", which was "git merge HEAD ...", has been deprecated for quite some time, and "git gui" was the last in-tree user of the syntax. This is finally fixed, so that we can move forward with the deprecation. Is this still true, given j6t's recent patch at http://public-inbox.org/git/e61cc267-a59b-3be1-29db-c49d56f52...@kdbg.org/T/ ? M.
Re: BUG: indent-with-non-tab always on
On 2016-08-15 12:55 PM, Marc Branchaud wrote: On 2016-08-15 11:00 AM, Philip Oakley wrote: From: "Marc Branchaud" On 2016-08-12 07:45 PM, Philip Oakley wrote: These are the quick fixes to Marc's comments to patches 5,6,11, and a consequential change to 12. Only the changed patches are included. Looks good to me -- no further comments! However, I still don't understand why git says 11/12 has "indent with spaces" problems. I'm guessing it's that the text is monospaced rather than tabbed, and it's flagging that. I'd noticed that it was highlighted in the Git gui (which I use to visualise both the diff, the message and the part after the three dashes at the same time). Actually, it looks like an indent-with-non-tab problem, which is supposed to be off by default. Git doesn't complain about the patch if I set core.whitespace = tabwidth=11 presumably because the patch uses 10 spaces to indent the offending lines. But explicitly disabling that check with core.whitespace = -indent-with-non-tab doesn't work. Unfortunately, I don't have time today to track this down. Gah, never mind -- didn't realize it was turned on in Documentation/.gitattributes. M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
BUG: indent-with-non-tab always on (was: Re: [PATCH v6 00/12] Update git revisions)
On 2016-08-15 11:00 AM, Philip Oakley wrote: From: "Marc Branchaud" On 2016-08-12 07:45 PM, Philip Oakley wrote: These are the quick fixes to Marc's comments to patches 5,6,11, and a consequential change to 12. Only the changed patches are included. Looks good to me -- no further comments! However, I still don't understand why git says 11/12 has "indent with spaces" problems. I'm guessing it's that the text is monospaced rather than tabbed, and it's flagging that. I'd noticed that it was highlighted in the Git gui (which I use to visualise both the diff, the message and the part after the three dashes at the same time). Actually, it looks like an indent-with-non-tab problem, which is supposed to be off by default. Git doesn't complain about the patch if I set core.whitespace = tabwidth=11 presumably because the patch uses 10 spaces to indent the offending lines. But explicitly disabling that check with core.whitespace = -indent-with-non-tab doesn't work. Unfortunately, I don't have time today to track this down. M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 00/12] Update git revisions
On 2016-08-12 07:45 PM, Philip Oakley wrote: These are the quick fixes to Marc's comments to patches 5,6,11, and a consequential change to 12. Only the changed patches are included. Looks good to me -- no further comments! However, I still don't understand why git says 11/12 has "indent with spaces" problems. M. Philip Oakley (12): doc: use 'symmetric difference' consistently doc: revisions - name the left and right sides doc: show the actual left, right, and boundary marks doc: revisions: give headings for the two and three dot notations doc: revisions: extra clarification of ^! notation effects doc: revisions: single vs multi-parent notation comparison doc: gitrevisions - use 'reachable' in page description doc: gitrevisions - clarify 'latter case' is revision walk doc: revisions - define `reachable` doc: revisions - clarify reachability examples doc: revisions: show revision expansion in examples doc: revisions: sort examples and fix alignment of the unchanged Documentation/gitk.txt | 2 +- Documentation/gitrevisions.txt | 6 +- Documentation/pretty-formats.txt | 2 +- Documentation/rev-list-options.txt | 4 +- Documentation/revisions.txt| 125 - 5 files changed, 88 insertions(+), 51 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 00/12] Update git revisions
On 2016-08-12 03:07 AM, Philip Oakley wrote: [2nd attempt : ISP troubles] This has grown like topsy from a little two patch series that tried to name the 2-dots notation [1] into this extended set of tweaks. Honestly, I start just trying point out something concrete, like misrendered headers, but then I figure since I'm sending a review comment anyway I might as well go in-depth on the rest of the patch. I think I'm sticking to substantive comments, but if I'm getting too picky please just swat me down! M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 11/12] doc: revisions: show revision expansion in examples
On 2016-08-12 03:07 AM, Philip Oakley wrote: The revisions examples show the revison arguments and the selected commits, but do not show the intermediate step of the expansion of the special 'range' notations. Extend the examples, including an all-parents multi-parent merge commit example. Sort the examples and fix the alignment for those unaffected in the next commit. Signed-off-by: Philip Oakley --- new Cc: Jakub Narębski --- Documentation/revisions.txt | 19 +-- 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt index 70864d5..ac7dd8e 100644 --- a/Documentation/revisions.txt +++ b/Documentation/revisions.txt @@ -326,16 +326,23 @@ Revision Range Summary as giving commit '' and then all its parents prefixed with '{caret}' to exclude them (and their ancestors). -Here are a handful of examples: +Here are a handful of examples using the Loeliger illustration above: + Args Expansion Selection I think "Result" would be better than "Selection" here. Also, shouldn't all the ^ in these examples be {caret}? (I likely just don't understand the rationale for using {caret} in some places and ^ in others...) DG H D D F G H I J D F ^G D H D ^D B E I J F B - B..C C - B...CG H D E B C + B..C = ^B C C + B...C = B ^F CG H D E B C ^D B C E I J F B C CI J F C - C^@ I J F - C^! C - F^! DG H D F + C^@= C^1 I have a mixed reaction to showing this "C^1" expansion, and the "B^1 B^2 B^3" one as well. I see the appeal of showing the parent notation, but really that was already explained to death in the first section. Here it's distracting. I think it's clearer for the reader to remove these expansions and just use the node names from the illustration. + = F I J F + B^@= B^1 B^2 B^3 + = D E F D G H E F I J + C^!= C ^C^1 I think this expansion might be better expressed as "C ^C^@". It'll be the same for "B^! = B ^B^@" as well, which demonstrates a nice consistency and also helps to emphasize the meaning of the ^@ notation. + = C ^F C + B^! = B ^B^1 ^B^2 ^B^3 + = B ^D ^E ^F B The layout of these last two lines doesn't match the others. They should be: B^!= B ^B^1 ^B^2 ^B^3 = B ^D ^E ^FB I see that the next patch fixes the layout of the unchanged examples, but it leaves these two unaligned. M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 06/12] doc: revisions: single vs multi-parent notation comparison
On 2016-08-12 03:07 AM, Philip Oakley wrote: Signed-off-by: Philip Oakley --- new Junio's final comment https://public-inbox.org/git/xmqqwpkq6b4d.fsf%40gitster.mtv.corp.google.com/ --- Documentation/revisions.txt | 4 1 file changed, 4 insertions(+) diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt index 0b5044d..934d071 100644 --- a/Documentation/revisions.txt +++ b/Documentation/revisions.txt @@ -284,6 +284,10 @@ The 'r1{caret}@' notation means all parents of 'r1'. 'r1{caret}!' notation includes commit 'r1' but excludes all of its parents. This is the single commit 'r1', if standalone. +While '{caret}' was about specifying a single commit parent, these +two notations consider all its parents. For example you can say +'HEAD{caret}2^@', however you cannot say 'HEAD{caret}@{caret}2'. That ^ should be {caret}, right? M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 05/12] doc: revisions: extra clarification of ^! notation effects
On 2016-08-12 03:07 AM, Philip Oakley wrote: Signed-off-by: Philip Oakley --- new Cc: Jakub Narębski https://public-inbox.org/git/578E4F4A.2020708%40gmail.com/ --- Documentation/revisions.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt index 3da0083..0b5044d 100644 --- a/Documentation/revisions.txt +++ b/Documentation/revisions.txt @@ -281,7 +281,8 @@ for naming a set that is formed by a commit and its parent commits. The 'r1{caret}@' notation means all parents of 'r1'. -'r1{caret}!' includes commit 'r1' but excludes all of its parents. +'r1{caret}!' notation includes commit 'r1' but excludes all of its parents. This sentence should start with "The". +This is the single commit 'r1', if standalone. That reads awkwardly to me. Perhaps By itself, this notation denotes the single commit 'r1'. ? M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 04/12] doc: revisions: give headings for the two and three dot notations
On 2016-08-12 03:07 AM, Philip Oakley wrote: While there, also break out the other shorthand notations and add a title for the revision range summary (which also appears in git-rev-parse, so keep it mixed case). We do not quote the notation within the headings as the asciidoc -> docbook -> groff man viewer toolchain, particularly the docbook-groff step, does not cope with two font changes, failing to return the heading font to bold after the quotation of the notation. Looks good --thanks! M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/8] doc: give headings for the two and three dot notations
On 2016-07-21 03:54 PM, Philip Oakley wrote: From: "Marc Branchaud" On 2016-07-20 05:10 PM, Philip Oakley wrote: While there, also break out the other shorthand notations and add a title for the revision range summary (which also appears in git-rev-parse, so keep it mixed case). Signed-off-by: Philip Oakley --- Documentation/revisions.txt | 58 - 1 file changed, 36 insertions(+), 22 deletions(-) diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt index 6e9cd41..5b37283 100644 --- a/Documentation/revisions.txt +++ b/Documentation/revisions.txt @@ -242,35 +242,49 @@ specifying a single revision with the notation described in the previous section means the set of commits reachable from that commit, following the commit ancestry chain. -To exclude commits reachable from a commit, a prefix '{caret}' -notation is used. E.g. '{caret}r1 r2' means commits reachable -from 'r2' but exclude the ones reachable from 'r1'. - -This set operation appears so often that there is a shorthand -for it. When you have two commits 'r1' and 'r2' (named according -to the syntax explained in SPECIFYING REVISIONS above), you can ask -for commits that are reachable from r2 excluding those that are reachable -from r1 by '{caret}r1 r2' and it can be written as 'r1..r2'. - -A similar notation 'r1\...r2' is called symmetric difference -of 'r1' and 'r2' and is defined as -'r1 r2 --not $(git merge-base --all r1 r2)'. -It is the set of commits that are reachable from either one of -'r1' (left side) or 'r2' (right side) but not from both. - -In these two shorthands, you can omit one end and let it default to HEAD. +Commit Exclusions +~ + +'{caret}' (caret) Notation:: + To exclude commits reachable from a commit, a prefix '{caret}' + notation is used. E.g. '{caret}r1 r2' means commits reachable + from 'r2' but exclude the ones reachable from 'r1'. + +Dotted Range Notations +~~ + +The '..' (two-dot) Range Notation:: + The '{caret}r1 r2' set operation appears so often that there is a shorthand + for it. When you have two commits 'r1' and 'r2' (named according + to the syntax explained in SPECIFYING REVISIONS above), you can ask + for commits that are reachable from r2 excluding those that are reachable + from r1 by '{caret}r1 r2' and it can be written as 'r1..r2'. + +The '...' (three dot) Symmetric Difference Notation:: + A similar notation 'r1\...r2' is called symmetric difference s/called/called the/ The wording is the original ;-) Can change. + of 'r1' and 'r2' and is defined as + 'r1 r2 --not $(git merge-base --all r1 r2)'. + It is the set of commits that are reachable from either one of + 'r1' (left side) or 'r2' (right side) but not from both. + +In these two shorthand notations, you can omit one end and let it default to HEAD. For example, 'origin..' is a shorthand for 'origin..HEAD' and asks "What did I do since I forked from the origin branch?" Similarly, '..origin' is a shorthand for 'HEAD..origin' and asks "What did the origin do since I forked from them?" Note that '..' would mean 'HEAD..HEAD' which is an empty range that is both reachable and unreachable from HEAD. -Two other shorthands for naming a set that is formed by a commit -and its parent commits exist. The 'r1{caret}@' notation means all -parents of 'r1'. 'r1{caret}!' includes commit 'r1' but excludes -all of its parents. +Special '{caret}' Shorthand Notations +~~ Sorry, but this header also does not render properly in the man page. Maybe just "Special {caret} Shorthand Notations"? (But read on!) rendered fine on the MYS2 man invocation - I had had to add the prefix to the quoted title to make it work. What went wrong for you? (I'll read on) Only the word "Special" is emphasized (in bold). The rest of the header is plain. I'm using asciidoc 8.6.9, so I'm likely suffering from the bug Peff identified. +Two other shorthands exist, particularly useful for merge commits, is +for naming a set that is formed by a commit and its parent commits. -To summarize: +The 'r1{caret}@' notation means all parents of 'r1'. + +'r1{caret}!' includes commit 'r1' but excludes all of its parents. My immediate thought upon reading this is "Why not just use 'r1'?" I think the answer is "This truncates the range." So, for example, "git lo
Re: [PATCH v4 4/8] doc: give headings for the two and three dot notations
On 2016-07-20 05:10 PM, Philip Oakley wrote: While there, also break out the other shorthand notations and add a title for the revision range summary (which also appears in git-rev-parse, so keep it mixed case). Signed-off-by: Philip Oakley --- Documentation/revisions.txt | 58 - 1 file changed, 36 insertions(+), 22 deletions(-) diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt index 6e9cd41..5b37283 100644 --- a/Documentation/revisions.txt +++ b/Documentation/revisions.txt @@ -242,35 +242,49 @@ specifying a single revision with the notation described in the previous section means the set of commits reachable from that commit, following the commit ancestry chain. -To exclude commits reachable from a commit, a prefix '{caret}' -notation is used. E.g. '{caret}r1 r2' means commits reachable -from 'r2' but exclude the ones reachable from 'r1'. - -This set operation appears so often that there is a shorthand -for it. When you have two commits 'r1' and 'r2' (named according -to the syntax explained in SPECIFYING REVISIONS above), you can ask -for commits that are reachable from r2 excluding those that are reachable -from r1 by '{caret}r1 r2' and it can be written as 'r1..r2'. - -A similar notation 'r1\...r2' is called symmetric difference -of 'r1' and 'r2' and is defined as -'r1 r2 --not $(git merge-base --all r1 r2)'. -It is the set of commits that are reachable from either one of -'r1' (left side) or 'r2' (right side) but not from both. - -In these two shorthands, you can omit one end and let it default to HEAD. +Commit Exclusions +~ + +'{caret}' (caret) Notation:: + To exclude commits reachable from a commit, a prefix '{caret}' + notation is used. E.g. '{caret}r1 r2' means commits reachable + from 'r2' but exclude the ones reachable from 'r1'. + +Dotted Range Notations +~~ + +The '..' (two-dot) Range Notation:: + The '{caret}r1 r2' set operation appears so often that there is a shorthand + for it. When you have two commits 'r1' and 'r2' (named according + to the syntax explained in SPECIFYING REVISIONS above), you can ask + for commits that are reachable from r2 excluding those that are reachable + from r1 by '{caret}r1 r2' and it can be written as 'r1..r2'. + +The '...' (three dot) Symmetric Difference Notation:: + A similar notation 'r1\...r2' is called symmetric difference s/called/called the/ + of 'r1' and 'r2' and is defined as + 'r1 r2 --not $(git merge-base --all r1 r2)'. + It is the set of commits that are reachable from either one of + 'r1' (left side) or 'r2' (right side) but not from both. + +In these two shorthand notations, you can omit one end and let it default to HEAD. For example, 'origin..' is a shorthand for 'origin..HEAD' and asks "What did I do since I forked from the origin branch?" Similarly, '..origin' is a shorthand for 'HEAD..origin' and asks "What did the origin do since I forked from them?" Note that '..' would mean 'HEAD..HEAD' which is an empty range that is both reachable and unreachable from HEAD. -Two other shorthands for naming a set that is formed by a commit -and its parent commits exist. The 'r1{caret}@' notation means all -parents of 'r1'. 'r1{caret}!' includes commit 'r1' but excludes -all of its parents. +Special '{caret}' Shorthand Notations +~~ Sorry, but this header also does not render properly in the man page. Maybe just "Special {caret} Shorthand Notations"? (But read on!) +Two other shorthands exist, particularly useful for merge commits, is +for naming a set that is formed by a commit and its parent commits. -To summarize: +The 'r1{caret}@' notation means all parents of 'r1'. + +'r1{caret}!' includes commit 'r1' but excludes all of its parents. My immediate thought upon reading this is "Why not just use 'r1'?" I think the answer is "This truncates the range." So, for example, "git log r1" shows you r1 and its ancestors, while "git log r1^!" only shows you r1. I think you should add this example, or something similar. But, really, this means that the notation is another "Commit Exclusion" and properly belongs in that section. That makes this "Special Notations" section rather thin. I suggest moving a slightly expanded ^@ description to a small subsection just before Commit Exclusions, and deleting the Special Notations section altogether. So add something like this: Commit Parents ~~ '{caret}@' Notation:: The 'r1{caret}@' notation means all parents of 'r1', excluding 'r1' itself. This smoothly re-introduces the notion of parents for readers who skipped to this section, and helps them make sense of the ^! notation. Plus there's no longer anything "special" about any of the syntax. + +Revision Range Summary +-- Sorry, but the man page renders this in all caps. I really think you
Re: [PATCH v3 4/8] doc: give headings for the two and three dot notations
On 2016-07-11 04:25 PM, Philip Oakley wrote: While there, also break out the other shorthand notations and add a title for the revision range summary (which also appears in git-rev-parse, so keep it mixed case). Signed-off-by: Philip Oakley --- Documentation/revisions.txt | 23 +-- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt index 79f6d03..1c59e87 100644 --- a/Documentation/revisions.txt +++ b/Documentation/revisions.txt @@ -242,35 +242,46 @@ specifying a single revision with the notation described in the previous section means the set of commits reachable from that commit, following the commit ancestry chain. +The '{caret}' (caret) notation +~~~ To exclude commits reachable from a commit, a prefix '{caret}' notation is used. E.g. '{caret}r1 r2' means commits reachable from 'r2' but exclude the ones reachable from 'r1'. All of these headings render poorly in the manpage, at least for me (Ubuntu 16.04). Only the first word appears in bold; the '-quoted text is not bold but underlined, and the rest of the header is plain. Also, I think calling this "The ^ notation" is confusing, because there's already an earlier paragraph on the "^" syntax. Maybe we don't need a header here? I only suggest that because I'm having trouble coming up with a nice alternative. "Commit Exclusion"? -This set operation appears so often that there is a shorthand +The '..' (two-dot) range notation +~ Perhaps "Range notation", to mirror the capitalization of "Symmetric Difference" in the next header? +The '{caret}r1 r2' set operation appears so often that there is a shorthand for it. When you have two commits 'r1' and 'r2' (named according to the syntax explained in SPECIFYING REVISIONS above), you can ask for commits that are reachable from r2 excluding those that are reachable from r1 by '{caret}r1 r2' and it can be written as 'r1..r2'. +The '...' (three dot) Symmetric Difference notation +~~~ A similar notation 'r1\...r2' is called symmetric difference of 'r1' and 'r2' and is defined as 'r1 r2 --not $(git merge-base --all r1 r2)'. It is the set of commits that are reachable from either one of 'r1' (Left side) or 'r2' (Right side) but not from both. -In these two shorthands, you can omit one end and let it default to HEAD. +In these two shorthand notations, you can omit one end and let it default to HEAD. For example, 'origin..' is a shorthand for 'origin..HEAD' and asks "What did I do since I forked from the origin branch?" Similarly, '..origin' is a shorthand for 'HEAD..origin' and asks "What did the origin do since I forked from them?" Note that '..' would mean 'HEAD..HEAD' which is an empty range that is both reachable and unreachable from HEAD. Unfortunately the new headings make it appear that this paragraph is exclusively part of the '...' notation section. Folks reading the '..' section are likely to skip it. I like the examples, though. I think it would be worthwhile to remove this paragraph and fold it explicitly into the '..' and '...' notation sections. So add something like this to the '..' section (only the first sentence here is new): Either r1 or r2 can be omitted, in which case HEAD is used as the default. For example, 'origin..' is a shorthand for 'origin..HEAD' and asks "What did I do since I forked from the origin branch?" Similarly, '..origin' is a shorthand for 'HEAD..origin' and asks "What did the origin do since I forked from them?" Note that '..' would mean 'HEAD..HEAD' which is an empty range that is both reachable and unreachable from HEAD. And also, add the same first sentence and a different example to the '...' section. Something like this: Either r1 or r2 can be omitted, in which case HEAD is used as the default. For example, 'origin...' is a shorthand for 'origin...HEAD' and asks "What have I and origin both done since I forked from the origin branch?" Note that 'origin...' and '...origin' ask the same question. +Additional '{caret}' Shorthand notations + Two other shorthands for naming a set that is formed by a commit -and its parent commits exist. The 'r1{caret}@' notation means all -parents of 'r1'. 'r1{caret}!' includes commit 'r1' but excludes -all of its parents. +and its parent commits exist. I think descriptions of ^@ and ^! should live under the main description of ^. That part already describes the numeric suffix, so describing a couple of special suffixes there seems like a natural fit. However, if you choose to keep this little section, you need to move the word "exist" earlier in the sentence: Two other shorthands exist for nami
Re: [PATCH v3 7/8] doc: revisions - define `reachable`
On 2016-07-11 04:25 PM, Philip Oakley wrote: Do not self-define `reachable`, which can lead to misunderstanding. Instead define `reachability` explictly. Signed-off-by: Philip Oakley --- Documentation/revisions.txt | 14 ++ 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt index 1c59e87..a3cd28b 100644 --- a/Documentation/revisions.txt +++ b/Documentation/revisions.txt @@ -237,10 +237,16 @@ SPECIFYING RANGES - History traversing commands such as `git log` operate on a set -of commits, not just a single commit. To these commands, -specifying a single revision with the notation described in the -previous section means the set of commits reachable from that -commit, following the commit ancestry chain. +of commits, not just a single commit. + +For these commands, +specifying a single revision, using the notation described in the +previous section, means the `reachable` set of commits of the given +commit. Better as "... means the set of commits `reachable` from the given commit." + +A commit's reachable set is the commit itself and the commits of +its ancestry chain. + s/of/in/ M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 0/5] Better ref summary alignment in "git fetch"
On 2016-07-01 12:03 PM, Nguyễn Thái Ngọc Duy wrote: v5 changes the substitute symbol from '$' to '*' in compact mode and makes sure long lines in compact mode will not make the remote ref column too big (it's far from perfect but I think it's still good to do). I think the first 4 patches are great. I have no opinion on the 5th patch, as I don't expect to use the compact format in any of its proposed forms (and I can't come up with an alternative). M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/6] git-fetch.txt: document fetch output
On 2016-06-04 11:11 PM, Nguyễn Thái Ngọc Duy wrote: This documents the ref update status of fetch. The structure of this output is defined in [1]. The ouput content is refined a bit in [2] [3] [4]. This patch is a copy from git-push.txt, modified a bit because the flag '-' means different things in push (delete) and fetch (tag update). PS. For code archaeologists, the discussion mentioned in [1] is probably [5]. [1] 165f390 (git-fetch: more terse fetch output - 2007-11-03) [2] 6315472 (fetch: report local storage errors ... - 2008-06-26) [3] f360d84 (builtin-fetch: add --prune option - 2009-11-10) [4] 0997ada (fetch: describe new refs based on where... - 2012-04-16) [5] http://thread.gmane.org/gmane.comp.version-control.git/61657 Signed-off-by: Nguyễn Thái Ngọc Duy --- Documentation/git-fetch.txt | 46 + 1 file changed, 46 insertions(+) diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt index efe56e0..a0d0539 100644 --- a/Documentation/git-fetch.txt +++ b/Documentation/git-fetch.txt @@ -99,6 +99,52 @@ The latter use of the `remote..fetch` values can be overridden by giving the `--refmap=` parameter(s) on the command line. +OUTPUT +-- + +The output of "git fetch" depends on the transport method used; this +section describes the output when fetch over the Git protocol (either s/fetch/fetching/ +locally or via ssh). + +The status of the fetch is output in tabular form, with each line +representing the status of a single ref. Each line is of the form: + +--- +-> () +--- () should really just be , as the () are part of the reason string. + +The status of up-to-date refs is shown only if --verbose option is s/if/if the/ Also, thanks for putting so much effort into this! I think having a fetch.output configuration setting is perfectly fine. This sort of thing is really a matter of personal taste, so having choices is good. M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] fetch: reduce duplicate in ref update status lines
On 2016-06-03 01:04 PM, Junio C Hamano wrote: Marc Branchaud writes: What if we detect when the full line exceeds the terminal width, and insert a newline after the remote ref and indent the -> to the same offset as its surrounding lines, like this: * [new branch] 2nd-index -> pclouds/2nd-index * [new branch] some-kind-of-long-ref-name -> pclouds/some-kind-of-long-ref-name * [new branch] 3nd-index -> pclouds/3nd-index I am OK with this format (not in the sense that I like it better than what the patch produces, but in the sense that I do not have strong preference either way). It may be hard to come up with a good heuristics to decide where on the display width "->" should come, though. I think aligning it with the -> on the other lines makes the most aesthetic sense. Are you worried that the right-hand-side ref might still wrap? I'm not too concerned about that -- there'll always be the possibility of a ref name that's longer than the terminal. +When `from` and `to` share a common suffix, the line could be +displayed in the form: + +--- + { -> } () If we go with this format, we'll need to document . The sentence above calls it "a common suffix", so instead of saying we can say perhaps? Or did you mean something more than that? I missed that, and although I think it's an adequate description I think most readers will miss it too. They eye tends to notice the syntax-description bits then skip down to the list of element descriptions to understand which bits mean what. My brain wants to find "suffix" in that list. Anyway, not a major issue, really. M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] fetch: reduce duplicate in ref update status lines
On 2016-06-03 07:08 AM, Nguyễn Thái Ngọc Duy wrote: When there are lots of ref updates, each has different name length, this will make it easier to look because the variable part is at the end. s/look/read/ For the record, I prefer the earlier stair-step format to this {xxx -> yyy}/foo stuff. Peff suggested that a two-pass approach might not be too bad, but had problems when he tried it with extra-long refs. Maybe those problems could be dealt with, and we could get a simple, aligned output? What if we detect when the full line exceeds the terminal width, and insert a newline after the remote ref and indent the -> to the same offset as its surrounding lines, like this: * [new branch] 2nd-index -> pclouds/2nd-index * [new branch] some-kind-of-long-ref-name -> pclouds/some-kind-of-long-ref-name * [new branch] 3nd-index -> pclouds/3nd-index --- Documentation/git-fetch.txt | 7 +++ builtin/fetch.c | 37 - t/t5510-fetch.sh| 4 ++-- t/t5526-fetch-submodules.sh | 26 +- 4 files changed, 58 insertions(+), 16 deletions(-) diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt index 18e733c..61c3bd1 100644 --- a/Documentation/git-fetch.txt +++ b/Documentation/git-fetch.txt @@ -113,6 +113,13 @@ representing the status of a single ref. Each line is of the form: -> () --- +When `from` and `to` share a common suffix, the line could be +displayed in the form: + +--- + { -> } () If we go with this format, we'll need to document . I'm not sure how to describe it succinctly, especially since it's not always used. Really there are two possible output formats: { -> } () -> () This is starting to feel too complex. M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] git-fetch.txt: document fetch output
On 2016-06-03 07:08 AM, Nguyễn Thái Ngọc Duy wrote: This documents the ref update status of fetch. The structure of this output is defined in [1]. The ouput content is refined a bit in [2] s/The ouput/The output/ [3] [4]. This patch is a copy from git-push.txt, modified a bit because the flag '-' means different things in push (delete) and fetch (tag update). We probably should unify the documents at some point in future. PS. For code archaeologists, the discussion mentioned in [1] is probably [5]. [1] 165f390 (git-fetch: more terse fetch output - 2007-11-03) [2] 6315472 (fetch: report local storage errors ... - 2008-06-26) [3] f360d84 (builtin-fetch: add --prune option - 2009-11-10) [4] 0997ada (fetch: describe new refs based on where... - 2012-04-16) [5] http://thread.gmane.org/gmane.comp.version-control.git/61657 --- Documentation/git-fetch.txt | 46 + 1 file changed, 46 insertions(+) diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt index efe56e0..18e733c 100644 --- a/Documentation/git-fetch.txt +++ b/Documentation/git-fetch.txt @@ -99,6 +99,52 @@ The latter use of the `remote..fetch` values can be overridden by giving the `--refmap=` parameter(s) on the command line. +OUTPUT +-- + +The output of "git fetch" depends on the transport method used; this What a mysterious statement! Does this tabular format actually change when fetching over HTTP? Maybe it's worth documenting the differences? +section describes the output when pushing over the Git protocol s/pushing/fetching/ +(either locally or via ssh). + +The status of the push is output in tabular form, with each line s/push/fetch/ +representing the status of a single ref. Each line is of the form: + +--- +-> () +--- + +The status of up-to-date refs is shown only if --verbose option is +used. + +flag:: + A single character indicating the status of the ref: +(space);; for a successfully fetched fast-forward; +`+`;; for a successful forced update; +`x`;; for a successfully deleted ref; I did a double-take here, until I remembered --prune. Maybe add "(when using the --prune option)"? M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] log: document the --decorate=auto option
On 2016-05-27 11:56 AM, Ramsay Jones wrote: Signed-off-by: Ramsay Jones --- Hi Junio, While reading an email from Linus earlier (RFC: dynamic "auto" date formats), I noticed that log.decorate was being set to 'auto'. Since I didn't recall that setting (even if it's easy to guess), I went looking for the documentation ... This should probably be marked RFC, since I haven't checked that the formatting is OK (I don't have the documentation toolchain installed these days). ATB, Ramsay Jones Documentation/config.txt | 5 - Documentation/git-log.txt | 8 +--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 53f00db..0707b3b 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1956,7 +1956,10 @@ log.decorate:: command. If 'short' is specified, the ref name prefixes 'refs/heads/', 'refs/tags/' and 'refs/remotes/' will not be printed. If 'full' is specified, the full ref name (including prefix) will be printed. - This is the same as the log commands '--decorate' option. + If 'auto' is specified, then if the output is going to a terminal, + the ref names are shown as if 'short' were given, otherwise no ref + names are shown. This is the same as the log commands '--decorate' s/commands/command's/ M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] fetch: better alignment in ref summary
On 2016-05-26 03:31 PM, Junio C Hamano wrote: Marc Branchaud writes: The fact that something is buried in some odd part of the ref tree is less relevant, IMO. If I'm using custom fetch refspecs or other oddities, I'll have that in the back of my head. But what I really care about is what ref I can use with commands like log and checkout. So, regarding Peff's examples: $ git fetch origin refs/pull/*/head:refs/remotes/pr/* Just show me the "pr/foo" refs. I know where things are coming from. Even if I configured that fetch refspec a long time ago, I don't need to see the exact mapping every time I fetch. That is only because you are used to seeing them. You cannot claim "I'll remember forever without seeing them" without actually experiencing not seeing them for a long time. I don't expect (or even want) to forever remember the mapping. It's a matter of context. When fetching, I'm interested in shiny new refs and I want to work with them. When configuring a remote repository, I'm interested in how to bring that repo's refs into my own repository. These are two distinct modes of thought, and I don't think fetch's output always needs to display the latter. Perhaps the --verbose switch could turn on showing how the remote refs get mapped. I can see how that would occasionally be useful for debugging fetch refspecs. I think the output should show the plain SHA values, since those are the only things the user can use to work with those refs. When they tell others how to reproduce what they did (or record what they did so that they can reproduce it later), they need what it is called at the remote end. Which is why I included the refnames in my proposal to Peff's second example. Really, the SHA and the remote's name for the SHA are the only meaningful things in this case. I would hesitate to go in the approach based on discarding information, as if it is the only way to shorten and clarify the output. Let's not do so before thinking things through to achieve the same while keeping the information we give to the users. I agree, this is not something to undertake lightly. But I've yet to be convinced of the utility of always showing the ref mapping during a fetch. I actually found fetch's output quite confusing when I first started using git. It's really not obvious that the thing on the left of the "->" is the remote's local name, especially since to see what was fetched one has to use the thing on the right-hand side -- which is obviously in a remote-specific namespace. (Admittedly, this was before checkout learned to search refs/remotes/ for things to check out.) M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] fetch: better alignment in ref summary
On 2016-05-26 01:42 PM, Junio C Hamano wrote: True. One of the entries in Marc's example is easily misread as "pclouds/2nd-index branch at its refs/heads/pclouds/2nd-index was fetched to its usual place", when Marc wanted to say "they had 2nd-index branch at refs/heads/2nd-index, and it was copied to our refs/remotes/pclouds/2nd-index". So even though we might be able to make sure it is unambiguous without "this -> that" ("->" is pronounced as 'became'), it is easily misread. Actually, I tend to just think of it as "this is a local name you can use to refer to the new thing that was just fetched." The left-hand side describes the thing being fetched (new or updated branch/tag/ref), and the right hand side shows how to locally refer to that thing. The fact that something is buried in some odd part of the ref tree is less relevant, IMO. If I'm using custom fetch refspecs or other oddities, I'll have that in the back of my head. But what I really care about is what ref I can use with commands like log and checkout. So, regarding Peff's examples: > $ git fetch origin refs/pull/*/head:refs/remotes/pr/* Just show me the "pr/foo" refs. I know where things are coming from. Even if I configured that fetch refspec a long time ago, I don't need to see the exact mapping every time I fetch. > $ git fetch origin refs/pull/77/head refs/pull/78/head Ah, now this is an odd case. Maybe there should be a different output format altogether for this one. The problem is that the remote refs being fetched are stored without any kind of local ref. (Commands like "git log FETCH_HEAD" only work with the last ref fetched, even though all the SHAs get added to the .git/FETCH_HEAD file. Maybe if git supported a "FETCH_HEAD@{x}" syntax...) I think the output should show the plain SHA values, since those are the only things the user can use to work with those refs. Maybe something like: From ... * origin:refs/pull/77/head abcd0123 * origin:refs/pull/78/head 453def00 (Not 100% sure about the "origin:" prefix...) M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] fetch: better alignment in ref summary
On 2016-05-22 09:59 PM, Duy Nguyen wrote: On Mon, May 23, 2016 at 7:58 AM, Junio C Hamano wrote: That is, I wonder if the above can become something like: From github.com:pclouds/git * [new branch] { -> pclouds/}2nd-index * [new branch] { -> pclouds/}3nd-index * [new branch] { -> pclouds/}file-watcher ... The above example borrows the idea used in diffstat label for renaming patch and I think you can design a better notataion, but a big point is that you can shorten the whole thing by not repeating the common part twice. The arrow aligns merely as a side effect of the shortening, taking advantage of the fact that most people fetch with "$their_prefix/*:$our_prefix/*" renaming refspec. I did think of that. My only concern is, with the new output I can't simply copy the ref name (e.g. pclouds/2nd-index) and use it to, say, examine with git-log. But I'm more of a "tab tab tab" person than "copy and paste", so I don't know how often people need to do that. Why do we need any kind of "->" at all? How about simply (with an update to "old-branch" for comparison to probably-more-common output): From github.com:pclouds/git cafed0c..badfeed pclouds/old-branch * [new branch] pclouds/2nd-index * [new branch] pclouds/3nd-index * [new branch] pclouds/file-watcher M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: /* compiler workaround */ - what was the issue?
On 2016-05-06 03:57 PM, Junio C Hamano wrote: Marc Branchaud writes: On 2016-05-06 02:54 PM, Junio C Hamano wrote: I wonder if can we come up with a short and sweet notation to remind futhre readers that this "initialization" is not initializing but merely squelching warnings from stupid compilers, and agree to use it consistently? Perhaps #define COMPILER_UNINITIALIZED_WARNING_INITIALIZER 0 or, for short-and-sweet #define CUWI 0 ? :) I get that smiley. Of course, right after I sent that I thought of #define SPURIOUS_COMPILER_RELATED_UNINITIALIZED_WARNING_INITIALIZER 0 or #define SCRUWI 0 Which we'd get to pronounce as "screwy". OK, I'll shut up now. M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: /* compiler workaround */ - what was the issue?
On 2016-05-06 02:54 PM, Junio C Hamano wrote: I wonder if can we come up with a short and sweet notation to remind futhre readers that this "initialization" is not initializing but merely squelching warnings from stupid compilers, and agree to use it consistently? Perhaps #define COMPILER_UNINITIALIZED_WARNING_INITIALIZER 0 or, for short-and-sweet #define CUWI 0 ? :) M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-cvsserver: fix duplicate words
On 2016-05-06 08:09 AM, Li Peng wrote: Fix duplicate words in comments. Signed-off-by: Li Peng --- git-cvsserver.perl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/git-cvsserver.perl b/git-cvsserver.perl index 02c0445..392e59e 100755 --- a/git-cvsserver.perl +++ b/git-cvsserver.perl @@ -1156,7 +1156,7 @@ sub prepDirForOutput # FUTURE: This would more accurately emulate CVS by sending # another copy of sticky after processing the files in that # directory. Or intermediate: perhaps send all sticky's for -# $seendirs after after processing all files. +# $seendirs after processing all files. } # update \n @@ -2824,7 +2824,7 @@ sub statecleanup } # Return working directory CVS revision "1.X" out -# of the the working directory "entries" state, for the given filename. +# of the working directory "entries" state, for the given filename. # This is prefixed with a dash if the file is scheduled for removal # when it is committed. sub revparse @@ -2935,7 +2935,7 @@ sub filecleanup return $filename; } -# Remove prependdir from the path, so that is is relative to the directory +# Remove prependdir from the path, so that is relative to the directory This one is a typo -- it should be "it is". Did you write a script to find these? :) (Also, I agree with Jeff that putting all of these changes into one patch would have been fine.) M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC: Supporting .git/hooks/$NAME.d/* && /etc/git/hooks/$NAME.d/*
On 2016-04-26 12:09 PM, Ævar Arnfjörð Bjarmason wrote: Basically you can look at patches to a project on a spectrum of: 1. You hacked something up locally 2. It's in someone's *.git repo as a POC 3. It's a third-party patch series used by a bunch of people 4. In an official release but documented as experimental 5. In an official release as a first-rate feature Something like an experimental.WHATEVER=bool flag provides #4. But the git project already does #4 without needing a special configuration tree. In fact, you ignored the git project's "pu" branch entirely. Once a feature gets onto the "next" branch, it's already much less experimental. If it needs to put the word "experimental" in its config settings, then maybe shouldn't leave the "pu" branch in the first place. I think aside from this feature just leaving these things undocumented really provides the worst of both worlds. Yes, I apologize. I did not mean that things should remain undocumented, only that if you're afraid of users harming themselves then you're better off not documenting settings than labelling them as experimental. Now you have some feature that's undocumented *because* you're not sure about it, but you can't ever be sure about it unless people actually use it, and if it's not documented at all effectively it's as visible as some third-party patch series. I.e. only people really involved in the project will ever use it. Slapping "experimental" on the configuration only serves to muddy the waters. Either the feature is good enough to be tried by normal users, or it isn't. If it isn't ready for normal users, let it cook in pu (or next) for a while longer. Git has got on just fine without having some special designation for not-ready-for-prime-time features, mostly because the development process lends itself naturally to gradually exposing things as they mature: topics move from the list to pu to next to master. (The string "experiment" only appears 16 times in all the release notes, which I think is something the git project can be proud of.) To me, designating part of the config tree as "experimental" enables sloppier practices, where a feature can be released with a bit less effort than might otherwise be acceptable, because it's clearly marked as experimental, and so anyone trying it out surely has the requisite bulletproof footwear. (I don't mean to imply that you or any other git contributor might slack off on any work you do for the project. It's more that the ability to easily designate something as experimental lowers the bar a bit, and makes it more tempting to release not-quite-ready features.) Far better to instead work on the feature until it's as ready as can be, and release it properly. In this particular case, for example, I think your proposed approach is perfectly fine and does not need to be designated as "experimental" in any way. With a reasonable "hooks.something" config variable to turn on directory support, what you've described sounds like a great feature. Sure, it may have bugs, it may have unintended consequences, it may not fulfill some odd corner cases. But that's true for almost everything. As with every other git feature, it'll be developed to the best of the project's abilities and released. Future patches are welcome. M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC: Supporting .git/hooks/$NAME.d/* && /etc/git/hooks/$NAME.d/*
On 2016-04-26 06:58 AM, Ævar Arnfjörð Bjarmason wrote: > > Makes sense to have an experimental.* config tree for git for stuff like this. I disagree. * If the point is to express some kind of warning to users, I think the community has been much better served by leaving experimental settings undocumented (or documented only in unmerged topic branches). It feels like an experimental.* tree is a doorway to putting experimental features in official releases, which seems odd considering that (IMHO) git has so far done very well with the carefully-planned-out integration of all sorts of features. * Part of the experiment is coming up with appropriate configuration knobs, including where those knobs should live. Often such considerations lead to a better implementation for the feature. Dumping things into an experimental.* tree would merely postpone that part of the feature's design. * Such a tree creates a flag day when the experimental feature eventually becomes a "real" feature. That'll annoy any early adopters. Sure, they *should* be prepared to deal with config tree bike-shedding, but still that extra churn seems unnecessary. M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Tabs in commit messages - de-tabify option in strbuf_stripspace()?
On 16-03-15 09:02 PM, Stefan Beller wrote: > On Tue, Mar 15, 2016 at 6:00 PM, Stefan Beller wrote: >> >> Instead of converting to whitespaces in Git, we could make use of the >> set_tabs capability for ttys and setup the terminal for having tabs align >> to 12,+8,+8,+8... > > Or rather read in the existing tabs configuration and shift it by a constant. Could this also help with diff output, where the leading + or - mars the indentation in a similar way? That opens a bit of a deeper problem, because not all the files in a single repo necessarily use the same tab size. M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] strtoul_ui: reject negative values
On 15-09-17 11:34 AM, Matthieu Moy wrote: > Marc Branchaud writes: > >>> --- a/git-compat-util.h >>> +++ b/git-compat-util.h >>> @@ -814,6 +814,9 @@ static inline int strtoul_ui(char const *s, int base, >>> unsigned int *result) >>> char *p; >>> >>> errno = 0; >>> + /* negative values would be accepted by strtoul */ >>> + if (strchr(s, '-')) >>> + return -1; >> >> I think this is broken, in that it doesn't match strtoul's normal behaviour, >> for strings like "1234-5678", no? > > The goal here is just to read a positive integer value. Rejecting > "1234-5678" is indeed a good thing. We already rejected it before my > patch by checking for p (AKA endptr for strtoul), as you noted below. > >> The test also doesn't work if the string has leading whitespace (" >> -5"). > > Why? It rejects any string that contain the character '-', regardless of > trailing spaces. Right, sorry. >>> ul = strtoul(s, &p, base); >>> if (errno || *p || p == s || (unsigned int) ul != ul) >>> return -1; >> >> Hmm, but we check *p here, so IIUC it's an error if the string has any >> trailing non-digits. Weird. > > strtoul_ui is more defensive than strtoul, by design. Fair enough, just not what I expected from a function with that name. M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] strtoul_ui: reject negative values
On 15-09-17 10:37 AM, Matthieu Moy wrote: > strtoul_ui uses strtoul to get a long unsigned, then checks that casting > to unsigned does not lose information and return the casted value. > > On 64 bits architecture, checking that the cast does not change the value > catches most errors, but when sizeof(int) == sizeof(long) (e.g. i386), > the check does nothing. Unfortunately, strtoul silently accepts negative > values, and as a result strtoul_ui("-1", ...) raised no error. > > This patch catches negative values before it's too late, i.e. before > calling strtoul. We still silently accept very large integers that wrap > to a valid "unsigned int". > > Reported-by: Max Kirillov > Signed-off-by: Matthieu Moy > --- > So, here's a proper patch (I mean, a band-aid patch, but properly > send ;-) ). > > It should be merged before Kartik's series (or inserted at the start > of the series) so that we get the fix before the test breakage. > > git-compat-util.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/git-compat-util.h b/git-compat-util.h > index f649e81..1df82fa 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -814,6 +814,9 @@ static inline int strtoul_ui(char const *s, int base, > unsigned int *result) > char *p; > > errno = 0; > + /* negative values would be accepted by strtoul */ > + if (strchr(s, '-')) > + return -1; I think this is broken, in that it doesn't match strtoul's normal behaviour, for strings like "1234-5678", no? The test also doesn't work if the string has leading whitespace (" -5"). > ul = strtoul(s, &p, base); > if (errno || *p || p == s || (unsigned int) ul != ul) > return -1; Hmm, but we check *p here, so IIUC it's an error if the string has any trailing non-digits. Weird. M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: proper remote ref namespaces
On 15-08-12 02:43 AM, Jacob Keller wrote: > Hello, > > Recently there was some discussion about git-notes and how we do not fetch > notes from remotes by default. The big problem with doing so is because > refs/remotes/* hierarchy is only setup for branches (heads), so we don't > have any clean location to put them. > > Around the time of git 1.8.0, Johan Herland made a proposal for remotes to > put all their refs in refs/remtoes/*, by moving heads into > refs/remotes/heads/* [1] Thanks for resurrecting this discussion. I feel this is a fundamental inconsistency in git's core design. Not a fatal flaw by any means, but something that keeps git from reaching its full potential. > In addition, his proposal was to include remote tags into > refs/remotes//tags and also refs/remotes//replace and > notes similarly. > > During this discussion there was many people who liked the idea, and > others who rejected it. The main rejection reason was two fold: > > (a) tags are "global" per project, so their namespace should be treated > global as it is now. > > The proposal's counter to this is that tags aren't guaranteed to be > global, because today two remotes you fetch might have tags that are the > same name with different pointers. This is currently hidden, and git > silently picks the tag it fetched first. > > (b) script compatibility, as changing the ref layout such that new git > can't work with old repository would be bad > > the counter to this, is that we make git smart enough to recognize old > remote format, and continue to work with it. Scripts which depend on this > layout will break, but that may not be such a huge concern. > > Personally, I think this proposal at least for heads, notes, replace, and > other remote refs we'd like to pull is very useful. I don't rightly know > the answer for tags. The linked discussion below covers several pages of > back and forth between a few people about which method is best. > > I like the idea of simplifying tags and branches and notes and others to > all fetch the same way. local stuff is in refs/heads or refs/notes and > remote stuff is (by default) in refs/remotes//tags etc > > But it does bring up some discussion as today we "auto follow" tags into > refs/tags, and it can get weird for tags since now "ambiguous" tags must > mean if there are tags of same name which point to different refs, The tags would be disambiguated by their remote name, e.g. "origin/tags/v5.6" vs. "hacker/tags/v5.6". The change would actually simplify tag auto-following. > and we'd need to teach a bunch of logic to the ref lookup code. Not a lot. Existing DWIMery already handles ambiguous branches, by preferring a local branch name over any remote ones. The only teaching that's really needed is to resolve shorthand like "origin/v5.6" to "refs/remotes/origin/tags/v5.6" (i.e. to look for anything matching "refs/remotes/origin/*/v5.6") but that doesn't seem very difficult. There is the question of how the user can even know if there's ambiguity. Aside from paying attention to "git fetch" output, I think we could extend "git tag" (and "git branch") in the case where the user specifies an existing tag (or branch). Right now both commands fail because the name already exists. All we need to do is extend that error message a bit, e.g.: > git tag v5.6 fatal: tag 'next' already exists as v5.6 -> a3a0e5d67d554a685abd897bc3ce4ffa4e74c812 origin/v5.6 remoteX/v5.6 -> 504346bbf9b02387b51f232f4db9c1860789b135 hacker/v5.6 -> fb4aa3533f81700789b3fb119e527410678e8d8d Here we see that our local v5.6, and hacker's v5.6, are unique, but origin and remoteX both have the same v5.6. Well, same-ish. I think those SHA IDs should be the things the tags point at, not the tag objects' IDs. This keeps things consistent between lightweight and annotated/signed tags. It does mean though that origin/v5.6 and remoteX/v5.6 might be different tag objects that happen to point at the same thing. I'm not sure the distinction is all that germane, and it's something that the user could disambiguate with "git tag -v" or "git show". > I am looking at ways to help git-notes be easier to use, so that we by > default fetch notes, and enable easier merge, since we'd have default > locations to merge from and to. This would make the sharing of notes a lot > easier, which is one of their primary sticking points.. you can't really > share them without *everyone* working to do it the same way you do. By > making a default policy, sharing becomes natural, and users can easily add > *public* notes to commits for things such as bug ids and other things > which are not discovered until after the commit is created. > > In addition, the easy ability to share replaces might also be helpful, > though IMHO not as valuable as git-notes. > > I think that the only logical refs layout is > "refs/remotes//(heads|tags|notes|replace)" > > and adding "refs/remote-notes" an
Re: A good time to pull from your gitk tree?
On 15-03-24 07:06 PM, Paul Mackerras wrote: > On Mon, Mar 23, 2015 at 12:03:37PM -0700, Junio C Hamano wrote: >> >> Is it a good time for me to pull from you, or do you recommend me to >> wait for a bit, expecting more? We'll go in the pre-release freeze >> soon-ish, so I thought I should ping. > > Now is a good time to pull from the usual place, thanks. Hi Paul, Is the usual place still git://ozlabs.org/~paulus/gitk ? The latest commit I get from there is commit c846920f23704ece225cc5b6c7566777fb561502 Author: Paul Mackerras Date: Sun Mar 15 17:25:02 2015 +1100 gitk: Update .po files Signed-off-by: Paul Mackerras It seems to be missing some of the changes you accepted earlier this week. Thanks! M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] gitk: Use translated version of "Command line" in getcommitlines.
Signed-off-by: Marc Branchaud --- I noticed this today. I think this change is needed for getcommitlines to work properly with translated gitk's. M. gitk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gitk b/gitk index 90419e3..fd5b50a 100755 --- a/gitk +++ b/gitk @@ -1442,7 +1442,7 @@ proc getcommitlines {fd inst view updating} { if {[string range $err 0 4] == "usage"} { set err "Gitk: error reading commits$fv:\ bad arguments to git log." - if {$viewname($view) eq "Command line"} { + if {$viewname($view) eq [mc "Command line"]} { append err \ " (Note: arguments to gitk are passed to git log\ to allow selection of commits to be displayed.)" -- 2.3.5 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2] gitk: Show the current view's name in the window title.
If the current view is the "Command line" view, show the command line arguments instead of the view name. Signed-off-by: Marc Branchaud --- This is v2 of my previous "Show the command-line revs in the window title" RFC patch. (I'm having trouble accessing gmane, or I'd include a link here.) This version incorporates Paul's feedback (thanks!) and handles view properly. M. gitk | 17 - 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/gitk b/gitk index b859879..90419e3 100755 --- a/gitk +++ b/gitk @@ -4034,6 +4034,19 @@ proc shellsplit {str} { return $l } +proc set_window_title {} { +global appname curview viewname vrevs +set rev [mc "All files"] +if {$curview ne 0} { + if {$viewname($curview) eq [mc "Command line"]} { + set rev [string map {"--gitk-symmetric-diff-marker" "--merge"} $vrevs($curview)] + } else { + set rev $viewname($curview) + } +} +wm title . "[reponame]: $rev - $appname" +} + # Code to implement multiple views proc newview {ishighlight} { @@ -4510,6 +4523,7 @@ proc showview {n} { } elseif {$numcommits == 0} { show_status [mc "No commits selected"] } +set_window_title } # Stuff relating to the highlighting facility @@ -6650,6 +6664,7 @@ proc show_status {msg} { global canv fgcolor clear_display +set_window_title $canv create text 3 3 -anchor nw -text $msg -font mainfont \ -tags text -fill $fgcolor } @@ -12393,7 +12408,7 @@ catch { } # wait for the window to become visible tkwait visibility . -wm title . "[reponame] - $appname" +set_window_title update readrefs -- 2.3.5 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: A good time to pull from your gitk tree?
On 15-03-24 07:06 PM, Paul Mackerras wrote: > On Mon, Mar 23, 2015 at 12:03:37PM -0700, Junio C Hamano wrote: >> >> Is it a good time for me to pull from you, or do you recommend me to >> wait for a bit, expecting more? We'll go in the pre-release freeze >> soon-ish, so I thought I should ping. > > Now is a good time to pull from the usual place, thanks. Paul, could you react to the gitk window-title patches I've posted: http://news.gmane.org/find-root.php?group=gmane.comp.version-control.git&article=262080 Thanks! M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] gitk: Show the rev(s) the user specified on the command line in the window title.
Signed-off-by: Marc Branchaud --- I often open multiple gitk windows in the same working directory to examine other branches or refs in the repo. This change allows me to distinguish which window is showing what. M. gitk | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/gitk b/gitk index b859879..379b13a 100755 --- a/gitk +++ b/gitk @@ -488,7 +488,7 @@ proc reset_pending_select {selid} { } proc getcommits {selid} { -global canv curview need_redisplay viewactive +global appname canv curview need_redisplay viewactive vrevs initlayout if {[start_rev_list $curview]} { @@ -498,6 +498,11 @@ proc getcommits {selid} { } else { show_status [mc "No commits selected"] } +set rev "$vrevs($curview)" +if {$rev eq ""} { + set rev "HEAD" +} +wm title . "[reponame]: $rev - $appname" } proc updatecommits {} { -- 2.3.3 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] gitk: Rearrange window title to be more conventional.
Signed-off-by: Marc Branchaud --- I did a bit of googling but couldn't find some standard that says the application name goes at the end of the title bar. But this is how all the browsers and other apps I use regularly do things. M. gitk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gitk b/gitk index 9a2daf3..b859879 100755 --- a/gitk +++ b/gitk @@ -12393,7 +12393,7 @@ catch { } # wait for the window to become visible tkwait visibility . -wm title . "$appname: [reponame]" +wm title . "[reponame] - $appname" update readrefs -- 2.3.3 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html