Re: js/mingw-http-ssl, was Re: What's cooking in git.git (Oct 2018, #05; Fri, 26)
Hi Junio, On 30 October 2018 11:07:39 GMT+05:30, Junio C Hamano wrote: >Eric Sunshine writes: >>> On platforms with recent cURL library, http.sslBackend >configuration >>> variable can be used to choose different SSL backend at runtime. >> >> s/choose/& a/ >> >>> The Windows port uses this mechanism to switch between OpenSSL >and >>> Secure Channel while talking over the HTTPS protocol. > >Thanks. > >By the way, I am beginning to like this phrasing quite a lot. It >encourages those with other ports like Linux and Mac to see if they >want a similar runtime switching by singling out Windows port, which >was what I wanted to achive with the original one, but I think the >updated text does so more effectively. The new phrasing seems to be reading quite better indeed. -- Sivaraam Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: git fetch behaves weirdely when run in a worktree
On 3 October 2018 00:13:06 GMT+05:30, Kaartic Sivaraam wrote: >Hi, > >Sorry for the delay. Got a little busy over the weekend. I seem to have >found the reason behind the issue in the mean time :-) > Oops, I forgot to mention there's more comments inline! BTW, is there an issue if .git/HEAD and .git/index are owned by root? The owners seem to have changed since I created the worktree possibly due to the cron job. Just wondering if it might cause some issues. >On Wed, 2018-09-26 at 10:05 -0700, Junio C Hamano wrote: >> Duy Nguyen writes: >> >> > On Wed, Sep 26, 2018 at 05:24:14PM +0200, Duy Nguyen wrote: >> > > On Wed, Sep 26, 2018 at 6:46 AM Kaartic Sivaraam >> > > wrote: >> > > > This is the most interesting part of the issue. I **did not** >run >> > > > 'git fetch ...' in between those cat commands. I was surprised >by >> > > > how the contents of FETCH_HEAD are changing without me spawning >any >> > > > git processes that might change it. Am I missing something >here? As >> > > > far as i could remember, there wasn't any IDE running that >might >> > > > automatically spawn git commands especially in that work >> > > > tree. Weird. >> > >> > Maybe something like this could help track down that rogue "git >fetch" >> > process (it's definitely _some_ process writing to the wrong file; >or >> > some file synchronization thingy is going on). You can log more of >> > course, but this is where FETCH_HEAD is updated. >> > >Thanks for the patch! It really helped me identify the issue. > >The actual culprit here doesn't seem to be Git itself. It was the "git- >prompt" bash plug-in[1] I was using. It seems to be spawning "git >fetch" for every command I type on shell. That answers why the >FETCH_HEAD was being updated even though I wasn't explicitly running >it. The git bash plug-in seems to be fetching changes for *all* the >upstream branches. That answers why there FETCH_HEAD was populated with >info about all the branches when I explicitly requested for the next >branch. I've also verified that `git fetch origin next` works as >intended (FETCH_HEAD has info only about orgin/next) when I turn-off >the plug-in which confirms that it's the culprit. A cursory search >found me a related issue[2] but I'm not sure if it's the exact same >one. > >I could identify the culprit only with the help of Duy's patch. Thanks >Duy! Sorry for not realising this earlier. I almost forgot I'm using it >as I've been accustomed to it a lot. > > >> Well, a background-ish thing could be some vendor-provided copy of >> Git that has nothing to do with what Kaartic would be compiling with >> this patch X-<. > >Fortunately, it wasn't the case here as the plug-in was using my >manually-built version of Git :-) > >Thanks for the help! > >Tag-line: Sometimes tools become part of our workflow so much that we >really don't realise their presence. It's an indication of a good tool >but we should be aware of suspecting them when an issue arises! >Something which I should have done to realise the issue ealier x-< > > >References: >[1]: https://github.com/magicmonty/bash-git-prompt >[2]: https://github.com/magicmonty/bash-git-prompt/issues/125 > >Thanks, >Sivaraam -- Sivaraam Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: git fetch behaves weirdely when run in a worktree
Hi, Sorry for the delay. Got a little busy over the weekend. I seem to have found the reason behind the issue in the mean time :-) On Wed, 2018-09-26 at 10:05 -0700, Junio C Hamano wrote: > Duy Nguyen writes: > > > On Wed, Sep 26, 2018 at 05:24:14PM +0200, Duy Nguyen wrote: > > > On Wed, Sep 26, 2018 at 6:46 AM Kaartic Sivaraam > > > wrote: > > > > This is the most interesting part of the issue. I **did not** run > > > > 'git fetch ...' in between those cat commands. I was surprised by > > > > how the contents of FETCH_HEAD are changing without me spawning any > > > > git processes that might change it. Am I missing something here? As > > > > far as i could remember, there wasn't any IDE running that might > > > > automatically spawn git commands especially in that work > > > > tree. Weird. > > > > Maybe something like this could help track down that rogue "git fetch" > > process (it's definitely _some_ process writing to the wrong file; or > > some file synchronization thingy is going on). You can log more of > > course, but this is where FETCH_HEAD is updated. > Thanks for the patch! It really helped me identify the issue. The actual culprit here doesn't seem to be Git itself. It was the "git- prompt" bash plug-in[1] I was using. It seems to be spawning "git fetch" for every command I type on shell. That answers why the FETCH_HEAD was being updated even though I wasn't explicitly running it. The git bash plug-in seems to be fetching changes for *all* the upstream branches. That answers why there FETCH_HEAD was populated with info about all the branches when I explicitly requested for the next branch. I've also verified that `git fetch origin next` works as intended (FETCH_HEAD has info only about orgin/next) when I turn-off the plug-in which confirms that it's the culprit. A cursory search found me a related issue[2] but I'm not sure if it's the exact same one. I could identify the culprit only with the help of Duy's patch. Thanks Duy! Sorry for not realising this earlier. I almost forgot I'm using it as I've been accustomed to it a lot. > Well, a background-ish thing could be some vendor-provided copy of > Git that has nothing to do with what Kaartic would be compiling with > this patch X-<. Fortunately, it wasn't the case here as the plug-in was using my manually-built version of Git :-) Thanks for the help! Tag-line: Sometimes tools become part of our workflow so much that we really don't realise their presence. It's an indication of a good tool but we should be aware of suspecting them when an issue arises! Something which I should have done to realise the issue ealier x-< References: [1]: https://github.com/magicmonty/bash-git-prompt [2]: https://github.com/magicmonty/bash-git-prompt/issues/125 Thanks, Sivaraam
Re: git fetch behaves weirdely when run in a worktree
Hi, I just wanted make a point a little more clear. See comment inline. On 24 September 2018 01:39:26 GMT+05:30, Kaartic Sivaraam wrote: > To add to that >confusion when I run > > $ cat $MAIN_WORKTREE/.git/worktrees/$BUILD_WORKTREE/FETCH_HEAD > >it seems to be printing the info about the remote refs once and then if >I run it again immediately nothing is printed. If I repeat it again, >info about remote refs is printed but this time the info about the >remote refs is printed thrice. This happens randomly. Sample output: > > 01:23 $ cat ../git/.git/worktrees/$BUILD_WORKTREE/FETCH_HEAD >53f9a3e157dbbc901a02ac2c73346d375e24978c not-for-merge branch 'maint' >of https://github.com/git/git >150f307afc13961b0322ad0e7205a7b193368586 not-for-merge branch 'master' >of https://github.com/git/git >01d371f741e6f0b0076d9ed942d99bdb23757eac not-for-merge branch 'next' of >https://github.com/git/git >7a81cbc028be113058e0b55062706ec6de62ed94 not-for-merge branch 'pu' of >https://github.com/git/git >722746685bce03f223ed75febe312495e6c139da not-for-merge branch 'todo' of >https://github.com/git/git > 01:23 $ cat ../git/.git/worktrees/$BUILD_WORKTREE/FETCH_HEAD > 01:23 $ cat ../git/.git/worktrees/$BUILD_WORKTREE/FETCH_HEAD > 01:23 $ cat ../git/.git/worktrees/$BUILD_WORKTREE/FETCH_HEAD >53f9a3e157dbbc901a02ac2c73346d375e24978c not-for-merge branch 'maint' >of https://github.com/git/git >150f307afc13961b0322ad0e7205a7b193368586 not-for-merge branch 'master' >of https://github.com/git/git >01d371f741e6f0b0076d9ed942d99bdb23757eac not-for-merge branch 'next' of >https://github.com/git/git >7a81cbc028be113058e0b55062706ec6de62ed94 not-for-merge branch 'pu' of >https://github.com/git/git >722746685bce03f223ed75febe312495e6c139da not-for-merge branch 'todo' of >https://github.com/git/git >53f9a3e157dbbc901a02ac2c73346d375e24978c not-for-merge branch 'maint' >of https://github.com/git/git >150f307afc13961b0322ad0e7205a7b193368586 not-for-merge branch 'master' >of https://github.com/git/git >01d371f741e6f0b0076d9ed942d99bdb23757eac not-for-merge branch 'next' of >https://github.com/git/git >7a81cbc028be113058e0b55062706ec6de62ed94 not-for-merge branch 'pu' of >https://github.com/git/git >722746685bce03f223ed75febe312495e6c139da not-for-merge branch 'todo' of >https://github.com/git/git >53f9a3e157dbbc901a02ac2c73346d375e24978c not-for-merge branch 'maint' >of https://github.com/git/git >150f307afc13961b0322ad0e7205a7b193368586 not-for-merge branch 'master' >of https://github.com/git/git >01d371f741e6f0b0076d9ed942d99bdb23757eac not-for-merge branch 'next' of >https://github.com/git/git >7a81cbc028be113058e0b55062706ec6de62ed94 not-for-merge branch 'pu' of >https://github.com/git/git >722746685bce03f223ed75febe312495e6c139da not-for-merge branch 'todo' of >https://github.com/git/git > 01:23 $ cat ../git/.git/worktrees/$BUILD_WORKTREE/FETCH_HEAD >53f9a3e157dbbc901a02ac2c73346d375e24978c not-for-merge branch 'maint' >of https://github.com/git/git >150f307afc13961b0322ad0e7205a7b193368586 not-for-merge branch 'master' >of https://github.com/git/git >01d371f741e6f0b0076d9ed942d99bdb23757eac not-for-merge branch 'next' of >https://github.com/git/git >7a81cbc028be113058e0b55062706ec6de62ed94 not-for-merge branch 'pu' of >https://github.com/git/git >722746685bce03f223ed75febe312495e6c139da not-for-merge branch 'todo' of >https://github.com/git/git > This is the most interesting part of the issue. I **did not** run 'git fetch ...' in between those cat commands. I was surprised by how the contents of FETCH_HEAD are changing without me spawning any git processes that might change it. Am I missing something here? As far as i could remember, there wasn't any IDE running that might automatically spawn git commands especially in that work tree. Weird. -- Sivaraam Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: git fetch behaves weirdely when run in a worktree
On 26 September 2018 03:10:17 GMT+05:30, Junio C Hamano wrote: > > That looks like fetching only the 'next' branch and nothing else to > me. > Interesting. > Perhaps your script is referring to a variable whose assignment is > misspelled and invoking > > git fetch $origin $branch > > and you are expecting the $branch variable to have string 'next' but > due to some bugs it is empty somehow? That explains why sometimes > (i.e. when $branch does not get the value you expect it to have) the > script fetches everything and some other times (i.e. when $branch > does get the right value) the script fetches only the named branch. Noce guees but I should have made this clear. The weirdness I described in my initial mail happens when I run "git fetch origin next" manually in the terminal. The script only helped me identity that there was an issue as it was automatically building the wrong version of Git. It was building and installing the current 'origin/maint' when I wrote it for building origin/next. I could easily identify the difference as 'origin/maint' was at v2.18 while 'origin/next' was at v2.19 when I notived this issue. Also the script doesn't depend on any variables for the branch name. I've hardcoded the 'next' branch into it. For the sake of reference, I've attached the script inline at the end of this mail. Note that I've sanitized the path in which the worktree exists as $COMMON_ROOT. I did not notice this weirdness in another PC. But, this happens in all the worktrees other than the main worktree in my laptop. I'm not sure what I'm doing weirdly that might have caused this issue. I'm not sure whether I mentioned this before I'm currently using a manually built version of Git. It was built from origin/next pointing at 01d371f741. But this also happens in Git v2.18.0 that comes via the pacakge manager of my operating system (Debian testing). -- Sivaraam #!/bin/sh # # A script for the cron job that automatically fetches # updates for the 'next' branch of Git and builds and # installs the binary from source. # # The build succeeds only if the config.mak is present # in the directory with appropriate configuration. # # The binary is installed into the default location if # a prefix is not specified in the config.mak file. # Bringing the binary into use is in the hands of the user. # Hint: A bash alias might help. GIT_NEXT_BUILD_AUTOMATE_DIR='$COMMON_ROOT/git-next-build-automate' LOG_FILE="$GIT_NEXT_BUILD_AUTOMATE_DIR/GIT-NEXT-INSTALLATION-LOG.txt" LOG_MSG_COMMON="installation of git 'next' build on $(date)" if cd "$GIT_NEXT_BUILD_AUTOMATE_DIR" then # Fetch the remote changes if git fetch origin next && git checkout -f FETCH_HEAD then if make install then GIT_NEXT_BUILD_STATUS=0 else GIT_NEXT_BUILD_STATUS=1 fi else GIT_NEXT_BUILD_STATUS=1 fi else GIT_NEXT_BUILD_STATUS=1 fi if test $GIT_NEXT_BUILD_STATUS -eq 0 then echo "SUCCESS: $LOG_MSG_COMMON" >>"$LOG_FILE" else echo "FAILURE: $LOG_MSG_COMMON" >>"$LOG_FILE" fi
Re: git fetch behaves weirdely when run in a worktree
On Mon, 2018-09-24 at 17:17 +0200, Duy Nguyen wrote: > On Sun, Sep 23, 2018 at 10:19 PM Kaartic Sivaraam > wrote: > > Yes, some bugs. It behaves correctly for me. There must be something > strange that triggers this. What's your "git worktree list" (iow > anything strange there, duplicate worktrees perhaps)? Nothing seems strange in the list. $ git worktree list $COMMON_ROOT/git 01d371f741 (detached HEAD) $COMMON_ROOT/git-next cfa73bbfcb (detached HEAD) $COMMON_ROOT/git-next-build-automate 01d371f741 (detached HEAD) $COMMON_ROOT/git-pu 363c5c06bb (detached HEAD) Note: I sanitized the path in which the git worktrees (including the main worktree) is present as $COMMON_ROOT. > Also please try > "git fetch" again with GIT_TRACE=1 and GIT_TRACE_SETUP=1. Hopefully we > could catch something with that. $ GIT_TRACE_SETUP=1 GIT_TRACE=1 git fetch origin next 23:10:26.049785 trace.c:377 setup: git_dir: $COMMON_ROOT/git/.git/worktrees/git-next-build-automate 23:10:26.049868 trace.c:378 setup: git_common_dir: $COMMON_ROOT/git/.git 23:10:26.049901 trace.c:379 setup: worktree: $COMMON_ROOT/git-next-build-automate 23:10:26.049922 trace.c:380 setup: cwd: $COMMON_ROOT/git-next-build-automate 23:10:26.049941 trace.c:381 setup: prefix: (null) 23:10:26.049955 git.c:415 trace: built-in: git fetch origin next 23:10:26.051033 run-command.c:637 trace: run_command: git-remote-https origin https://github.com/git/git.git 23:10:28.366526 run-command.c:637 trace: run_command: git rev-list --objects --stdin --not --all --quiet 23:10:28.400979 run-command.c:637 trace: run_command: git rev-list --objects --stdin --not --all --quiet 23:10:28.402745 trace.c:377 setup: git_dir: $COMMON_ROOT/git/.git/worktrees/git-next-build-automate 23:10:28.402787 trace.c:378 setup: git_common_dir: $COMMON_ROOT/git/.git 23:10:28.402793 trace.c:379 setup: worktree: $COMMON_ROOT/git-next-build-automate 23:10:28.402798 trace.c:380 setup: cwd: $COMMON_ROOT/git-next-build-automate 23:10:28.402802 trace.c:381 setup: prefix: (null) 23:10:28.402815 git.c:415 trace: built-in: git rev-list --objects --stdin --not --all --quiet >From https://github.com/git/git * branch next -> FETCH_HEAD 23:10:28.437350 run-command.c:1553 run_processes_parallel: preparing to run up to 1 tasks 23:10:28.437481 run-command.c:1585 run_processes_parallel: done 23:10:28.437763 run-command.c:637 trace: run_command: git gc --auto 23:10:28.439608 trace.c:377 setup: git_dir: $COMMON_ROOT/git/.git/worktrees/git-next-build-automate 23:10:28.439655 trace.c:378 setup: git_common_dir: $COMMON_ROOT/git/.git 23:10:28.439667 trace.c:379 setup: worktree: $COMMON_ROOT/git-next-build-automate 23:10:28.439677 trace.c:380 setup: cwd: $COMMON_ROOT/git-next-build-automate 23:10:28.439687 trace.c:381 setup: prefix: (null) 23:10:28.439699 git.c:415 trace: built-in: git gc --auto HTH, Sivaraam
git fetch behaves weirdely when run in a worktree
Hi, I was actually trying to automae the building and installation of Git source code to reduce my burden. I tried to automate it with the help of a script that runs daily via cron and a separate worktree used only by the build script.y run The script typically fetches new changes for the next branch by running the following in the build worktree (which is not the main worktree): $ git fetch origin next I thought that would result in FETCH_HEAD pointing to the latest changes for origin/next if the command succeeded. Unfortunately, it seems to be behaving weirdely when run in a worktree. It sems to be behaving as if I ran 'git fetch origin'. To add to that confusion when I run $ cat $MAIN_WORKTREE/.git/worktrees/$BUILD_WORKTREE/FETCH_HEAD it seems to be printing the info about the remote refs once and then if I run it again immediately nothing is printed. If I repeat it again, info about remote refs is printed but this time the info about the remote refs is printed thrice. This happens randomly. Sample output: 01:23 $ cat ../git/.git/worktrees/$BUILD_WORKTREE/FETCH_HEAD 53f9a3e157dbbc901a02ac2c73346d375e24978c not-for-merge branch 'maint' of https://github.com/git/git 150f307afc13961b0322ad0e7205a7b193368586not-for-merge branch 'master' of https://github.com/git/git 01d371f741e6f0b0076d9ed942d99bdb23757eacnot-for-merge branch 'next' of https://github.com/git/git 7a81cbc028be113058e0b55062706ec6de62ed94not-for-merge branch 'pu' of https://github.com/git/git 722746685bce03f223ed75febe312495e6c139danot-for-merge branch 'todo' of https://github.com/git/git 01:23 $ cat ../git/.git/worktrees/$BUILD_WORKTREE/FETCH_HEAD 01:23 $ cat ../git/.git/worktrees/$BUILD_WORKTREE/FETCH_HEAD 01:23 $ cat ../git/.git/worktrees/$BUILD_WORKTREE/FETCH_HEAD 53f9a3e157dbbc901a02ac2c73346d375e24978c not-for-merge branch 'maint' of https://github.com/git/git 150f307afc13961b0322ad0e7205a7b193368586not-for-merge branch 'master' of https://github.com/git/git 01d371f741e6f0b0076d9ed942d99bdb23757eacnot-for-merge branch 'next' of https://github.com/git/git 7a81cbc028be113058e0b55062706ec6de62ed94not-for-merge branch 'pu' of https://github.com/git/git 722746685bce03f223ed75febe312495e6c139danot-for-merge branch 'todo' of https://github.com/git/git 53f9a3e157dbbc901a02ac2c73346d375e24978cnot-for-merge branch 'maint' of https://github.com/git/git 150f307afc13961b0322ad0e7205a7b193368586not-for-merge branch 'master' of https://github.com/git/git 01d371f741e6f0b0076d9ed942d99bdb23757eacnot-for-merge branch 'next' of https://github.com/git/git 7a81cbc028be113058e0b55062706ec6de62ed94not-for-merge branch 'pu' of https://github.com/git/git 722746685bce03f223ed75febe312495e6c139danot-for-merge branch 'todo' of https://github.com/git/git 53f9a3e157dbbc901a02ac2c73346d375e24978cnot-for-merge branch 'maint' of https://github.com/git/git 150f307afc13961b0322ad0e7205a7b193368586not-for-merge branch 'master' of https://github.com/git/git 01d371f741e6f0b0076d9ed942d99bdb23757eacnot-for-merge branch 'next' of https://github.com/git/git 7a81cbc028be113058e0b55062706ec6de62ed94not-for-merge branch 'pu' of https://github.com/git/git 722746685bce03f223ed75febe312495e6c139danot-for-merge branch 'todo' of https://github.com/git/git 01:23 $ cat ../git/.git/worktrees/$BUILD_WORKTREE/FETCH_HEAD 53f9a3e157dbbc901a02ac2c73346d375e24978c not-for-merge branch 'maint' of https://github.com/git/git 150f307afc13961b0322ad0e7205a7b193368586not-for-merge branch 'master' of https://github.com/git/git 01d371f741e6f0b0076d9ed942d99bdb23757eacnot-for-merge branch 'next' of https://github.com/git/git 7a81cbc028be113058e0b55062706ec6de62ed94not-for-merge branch 'pu' of https://github.com/git/git 722746685bce03f223ed75febe312495e6c139danot-for-merge branch 'todo' of https://github.com/git/git 'git fetch ' behaves correctly in the main worktree. Why is this weirdness happening when run in other worktrees? Why isn't 'git fetch not fetching the changes for just the specified branch? Am I missing something? -- Sivaraam
Re: [ANNOUNCE] Git v2.19.0-rc0
On Thu, 2018-08-23 at 06:26 -0400, Derrick Stolee wrote: > > Around the time that my proposed approaches were getting vetoed for > alignment issues, I figured I was out of my depth here. I reached out to > Daniel Lemire (of EWAH bitmap fame) on Twitter [1]. His blog is full of > posts of word-based approaches to different problems, so I thought he > might know something off the top of his head that would be applicable. > His conclusion (after looking only a short time) was to take a 'hasheq' > approach [2] like Peff suggested [3]. Since that requires auditing all > callers of hashcmp to see if hasheq is appropriate, it is not a good > solution for 2.19 but (in my opinion) should be evaluated as part of the > 2.20 cycle. > That was an interesting blog post, indeed. It had an interesting comments section. One comment especially caught my eyes was [a]: "So the way gcc (and maybe clang) handles this is specifically by recognizing memcmp and checking whether a only a 2-way result is needed and then essentially replacing it with a memcmp_eq call. ..." I find this to be an interesting note. It seems GCC does optimize when we clearly indicate that we use the result of the memcmp as a boolean. So would that help in anyway? Maybe it would help in writing a `hasheq` method easily? I'm not sure. > [1] https://twitter.com/stolee/status/1032312965754748930 > > [2] > https://lemire.me/blog/2018/08/22/avoid-lexicographical-comparisons-when-testing-for-string-equality/ > > [3] > https://public-inbox.org/git/20180822030344.ga14...@sigill.intra.peff.net/ > > [4] > https://public-inbox.org/git/7ea416cf-b043-1274-e161-85a8780b8...@gmail.com/ [a]: https://lemire.me/blog/2018/08/22/avoid-lexicographical-comparisons-when-testing-for-string-equality/#comment-344073 -- Sivaraam
Re: [PATCH] Makefile: enable DEVELOPER by default
On Mon, 2018-08-06 at 13:02 -0400, Randall S. Becker wrote: > > Any idea when this is going to be in an official release, and exactly > what the settings will be for "Not Developer". I assume DEVELOPER=0 > and DEVOPTS=error, which is the current behaviour, correct? I am the > platform maintainer for HPE NonStop and need to make sure I'm not > packaging DEV builds to anyone, since I'm the only one doing this for > the platform. It's another hoop, but hopefully not a bad one. The > question is the best place to set this, assuming we are using Jenkins > for our builds, and I'd rather keep the existing config.mak.uname the > same, since at least it seems stable. Just a FYI and in case you aren't aware, you could create a "config.mak" to store your custom configurations. You can be sure it's used due to the following Makefile part: ... include config.mak.uname -include config.mak.autogen -include config.mak ... It's just not a hard dependency. Hope that helps, Sivaraam
Re: What's cooking in git.git (Aug 2018, #06; Wed, 29)
On 30 August 2018 21:41:35 GMT+05:30, Elijah Newren wrote: >That makes sense. I'm not sure I can concisely convey all the right >points, but here's a stab at rewording: > >Recent addition of "directory rename" heuristics to the >merge-recursive backend makes the command susceptible to false >positives and false negatives. In the context of "git am -3", which >does not know about surrounding unmodified paths and thus cannot >inform the merge machinery about the full trees involved, this risk is >particularly severe. As such, the heuristic is disabled for "git am >-3" to keep the machinery "more stupid but predictable". FWIW, this sounds better to me than the original. -- Sivaraam Sent from my Android device with K-9 Mail. Please excuse my brevity.
Doc/SubmittingPatches: re-phrashing a sentence about alternate solutions (was Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv")
On Sun, 2018-06-17 at 14:00 -0400, Eric Sunshine wrote: > Whether or not to talk about alternate solutions in the commit message > is a judgment call. Same for deciding what belongs in the commit > message proper and what belongs in the "commentary" section of a > patch. A patch author should strive to convey the problem succinctly > in the commit message, to not overload the reader with unnecessary (or > confusing) information, while, at the same time, not be sparing with > information which is genuinely needed to understand the problem and > solution. > > Often, this can be done without talking about alternatives; often even > without spelling out the solution in detail or at all since the > solution may be "obvious", given a well-written problem description. > Complex cases, or cases in which multiple solutions may be or seem > valid, on the other hand, might warrant talking about those alternate > solutions, so we probably don't want to drop that bullet point. Well explained, thanks. (Thinking out loud, it might be even nice to including the above paragraphs into Documentation/SubmittingPatches as I find it to be more "humane" than the terse bullets. But I refrained from doing so as the document is already a bit too-long ;-) > Perhaps, instead, it can be re-worded a bit to make it sound something > other than mandatory (but I can't think of a good way to phrase it; > maybe you can?). How about the following patch? (warning: patch only for discussion purposes, might be white-space broken). It might be superfluous, though. diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index a1d0feca3..565bc4397 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -128,7 +128,7 @@ The body should provide a meaningful commit message, which: . justifies the way the change solves the problem, i.e. why the result with the change is better. -. alternate solutions considered but discarded, if any. +. alternate solutions considered but discarded, where necessary. [[imperative-mood]] Describe your changes in imperative mood, e.g. "make xyzzy do frotz" Regards, Sivaraam
Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv"
On Friday 15 June 2018 01:13 PM, Eric Sunshine wrote: > On Fri, Jun 15, 2018 at 2:58 AM Simon Ruderich wrote: >> On Thu, Jun 14, 2018 at 10:25:03PM -0400, Eric Sunshine wrote: >>> This patch is extra noisy due to the indentation change. Viewing it with >>> "git diff -w" helps. An alternative to re-indenting would have been to >>> "undefine NEEDS_LIBICONV", however, 'undefine' was added to GNU make in >>> 3.82 but MacOS is stuck on 3.81 (from 2006) so 'undefine' was avoided. >> >> Should we put the part about MacOS's make into the commit >> message? Seems like relevant information for future readers. > > No. The bit of commentary mentioning MacOS's very old 'make' was just > talking about a possible alternate way of implementing the change. > That alternative was not chosen, so talking about old 'make' in the > commit message would be confusing for readers. Interesting. Documentation/SubmittinPatches reads: The body should provide a meaningful commit message, which: ... ... . alternate solutions considered but discarded, if any. The consensus has changed, maybe? In which case, should we remove that statement from there? -- Sivaraam QUOTE: “The three principal virtues of a programmer are Laziness, Impatience, and Hubris.” - Camel book Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities in the place I live as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
Re: [PATCH] submodule: fix NULL correctness in renamed broken submodules
On Thursday 14 June 2018 11:01 PM, Stefan Beller wrote: > While at it, make sure we only attempt to load the submodule if a git > directory of the submodule is found as default_name_or_path will return > NULL in case the git directory cannot be found. I found this a little hard to read. Maybe it could be sentence could be shrinked a little. Possibly, While at it, make sure we only attempt to load the submodule if a git directory of the submodule is found. I guess the other part of the sentence doesn't make much sense in the log message. Maybe it could be an in-code comment. Speaking of in-code comment, the following would also fit there, wouldn't it? > Note that passing NULL > to submodule_from_name is just a semantic error, as submodule_from_name > accepts NULL as a value, but then the return value is not the submodule > that was asked for, but some arbitrary other submodule. (Cf. 'config_from' > in submodule-config.c: "If any parameter except the cache is a NULL > pointer just return the first submodule. Can be used to check whether > there are any submodules parsed.") > -- Sivaraam QUOTE: “The three principal virtues of a programmer are Laziness, Impatience, and Hubris.” - Camel book Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities in the place I live as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
[PATCH v2] t3200: clarify description of --set-upstream test
Support for the --set-upstream option was removed in 52668846ea (builtin/branch: stop supporting the "--set-upstream" option, 2017-08-17). The change did not completely remove the command due to an issue noted in the commit's log message. So, a test was added to ensure that a command which uses the '--set-upstream' option fails instead of silently acting as an alias for the '--set-upstream-to' option due to option parsing features. To avoid confusion, clarify that the option is disabled intentionally in the corresponding test description. The test is expected to be around as long as we intentionally fail on seeing the '--set-upstream' option which in turn we expect to do for a period of time after which we can be sure that existing users of '--set-upstream' are aware that the option is no longer supported. Signed-off-by: Kaartic Sivaraam --- Changes since v1: - update the test description as suggested by Junio - updated the commit message to be clear about period upto which we should be testing this. t/t3200-branch.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 6c0b7ea4a..e98dbfc1a 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -884,7 +884,7 @@ test_expect_success 'test --unset-upstream on a particular branch' ' test_must_fail git config branch.my14.merge ' -test_expect_success '--set-upstream fails' ' +test_expect_success 'disabled option --set-upstream fails' ' test_must_fail git branch --set-upstream origin/master ' -- 2.18.0.rc1.242.g61856ae69
Re: [PATCH] t3200: clarify description of --set-upstream test
On Thursday 14 June 2018 11:13 PM, Junio C Hamano wrote: > It is technically correct to call --set-upstream "unsupported", but > the reason why we want to see it fail is not because it is > unsupported, but because we actively interfere with the usual > "unique prefix" logic parse-options API gives its users and make it > not to trigger the longer-and-unique --set-upstream-to logic. > That sounds right. >> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh >> index 6c0b7ea4a..d14de82ba 100755 >> --- a/t/t3200-branch.sh >> +++ b/t/t3200-branch.sh >> @@ -884,7 +884,7 @@ test_expect_success 'test --unset-upstream on a >> particular branch' ' >> test_must_fail git config branch.my14.merge >> ' >> >> -test_expect_success '--set-upstream fails' ' >> +test_expect_success 'unsupported option --set-upstream fails' ' > > In other words, I am wondering if s/unsupported/disabled/ makes it > even more clear what is going on here. > I guess it would :-) Thanks for the better wording. I actually thought of asking for a better wording for the test message while sending the patch but somehow forgot to mention it. It seems I've got better wordings, regardless. I'll send a v2 with the correction. I'm still open to an alternative test description in case that make things even more clearer :-) Thanks, Sivaraam QUOTE: “The three principal virtues of a programmer are Laziness, Impatience, and Hubris.” - Camel book Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities in the place I live as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
[PATCH] t3200: clarify description of --set-upstream test
Support for the --set-upstream option was removed in 52668846ea (builtin/branch: stop supporting the "--set-upstream" option, 2017-08-17). The change did not completely remove the command due to an issue noted in the commit's log message. So, a test was added to ensure that a command which uses the '--set-upstream' option fails and doesn't silently act as an alias for the '--set-upstream-to' option due to option parsing features. To avoid confusion, clarify that the option is an unsupported one in the corresponding test description. Signed-off-by: Kaartic Sivaraam --- t/t3200-branch.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 6c0b7ea4a..d14de82ba 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -884,7 +884,7 @@ test_expect_success 'test --unset-upstream on a particular branch' ' test_must_fail git config branch.my14.merge ' -test_expect_success '--set-upstream fails' ' +test_expect_success 'unsupported option --set-upstream fails' ' test_must_fail git branch --set-upstream origin/master ' -- 2.18.0.rc1.242.g61856ae69
Re: BUG: submodule code prints '(null)'
On Tuesday 05 June 2018 09:01 PM, Duy Nguyen wrote: > I'll leave it to submodule people to fix this :) > I'm Ccing the only one I know to gain attention. -- Sivaraam QUOTE: “The three principal virtues of a programmer are Laziness, Impatience, and Hubris.” - Camel book Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities in the place I live as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
Re: [PATCH] t3200-branch.sh: use "--set-upstream-to" in test
On Tuesday 05 June 2018 04:54 PM, SZEDER Gábor wrote: > > Though arguably the test name could be more descriptive and tell why > it should fail. > That's arguable, indeed. I was about to send a patch that gives a better description for the test. I didn't do it as I started wondering, Is it even worth testing whether a removed option fails? Is this done for other options that have been removed in the past? Should we just remove the test completely? -- Sivaraam QUOTE: “The three principal virtues of a programmer are Laziness, Impatience, and Hubris.” - Camel book Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities in the place I live as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
Re: [PATCH] branch: issue "-l" deprecation warning after pager starts
On Friday 25 May 2018 08:10 AM, Jeff King wrote: > Subject: [PATCH] branch: customize "-l" warning in list mode > > People mistakenly use "git branch -l", thinking that it > triggers list mode. It doesn't, but the lack of non-option > arguments in that command does (and the "-l" becomes a > silent noop). > > Since afc968e579 (branch: deprecate "-l" option, 2018-03-26) > > we've warned that "-l" is going away. But the warning text > is primarily aimed at people who _meant_ to use "-l", as in > "git branch -l foo". People who mistakenly said "git branch > -l" may be left puzzled. > So, this patch is to improve the user experience of people who use "git branch -l" for listing and not for the people who forget to give a new branch name argument for "-l". In that case, this makes sense. BTW, I hope people don't start wondering why "git branch -d" doesn't trigger list mode ;-) > + warning("the '-l' option is an alias for > '--create-reflog' and"); > + warning("has no effect in list mode. This option will > soon be"); > + warning("removed and you should omit it (or use > '--list' instead)."); I suppose s/alias/deprecated alias/ makes the point that '-l' will be removed more stronger. -- Sivaraam signature.asc Description: This is a digitally signed message part
Re: [PATCH] branch: issue "-l" deprecation warning after pager starts
On Friday 25 May 2018 01:01 AM, Jeff King wrote: > On Thu, May 24, 2018 at 03:22:14PM -0400, Jeff King wrote: > > Hmm, actually, I suppose the true value of the warning is to help people > doing "git branch -l foo", and it would still work there. The "more > extreme" from your suggested patch would only affect "branch -l". > > Still, I think I prefer the gentler version that we get by keeping it as > a warning even in the latter case. > I never wanted to suppress the warning message in the latter case. I just wanted to avoid listing the branches. Actually the patch I sent in one the previous threads[1] that avoids listing the branches has the following behaviour, $ git branch -l warning: the '-l' alias for '--create-reflog' is deprecated; warning: it will be removed in a future version of Git usage: git branch [] [-r | -a] [--merged | --no-merged] or: git branch [] [-l] [-f] [] or: git branch [] [-r] (-d | -D) ... or: git branch [] (-m | -M) [] or: git branch [] (-c | -C) [] or: git branch [] [-r | -a] [--points-at] or: git branch [] [-r | -a] [--format] So, the warning message isn't lost. It just prevents the listing of branches. Wait, maybe I'm misunderstanding what you mean by "warning". You're probably meaning something related to the way Git exits in both cases. With your patch "git branch -l" prints a warning, lists the branches and has an exit status of 0. With my patch it prints the warning, the usage specifications with exit status 128. In that case, I still don't think it's bad to turn "git branch -l" into an error now as it's been incorrect for a long time now and it's not wrong if we correct it now. Anyways, if you think it mustn't turn into an error now and only in the next stage, a suggestion follows in another thread. [1]: https://public-inbox.org/git/1527174618.10589.4.ca...@gmail.com/ -- Sivaraam QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - John Sonmez Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities in the place I live as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
jk/branch-l-0-deprecation (was Re: What's cooking in git.git (May 2018, #02; Thu, 17))
On Thursday 17 May 2018 07:06 PM, Jeff King wrote: > But because git-branch does not kick in the pager until later > (because it only wants to do it for list-mode), that happens _after_ > we've emitted the message. > I observe exactly the consequence of this behaviour. First, the error is emitted and then the pager kicks in to list the branches. > On the other hand, I'm not sure this is that big a deal. The point of > the deprecation warning is to catch people who are actually trying to > use "-l" as "--create-reflog", and that case does not page. The people > doing "git branch -l" are actually getting what they want eventually, > which is to turn it into "--list". In the interim step where it becomes > an unknown option, they'll get a hard error. I just thought we wouldn't want to surprise/confuse users who try to use "git branch -l" with the warning message about "create reflog" along-side the list of branches. That would just add to the confusion. So, I thought we should error out when users do "git branch -l" instead. Something like the following should help us prevent "git branch -l" from listing branch names and might also prevent the confusion. diff --git a/builtin/branch.c b/builtin/branch.c index 452742fec..f3c5181bb 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -672,7 +672,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, builtin_branch_usage, 0); - if (!delete && !rename && !copy && !edit_description && !new_upstream && !unset_upstream && argc == 0) + if (!delete && !rename && !copy && !edit_description && !new_upstream && !unset_upstream && !reflog && argc == 0) list = 1; if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr || -- Kaartic signature.asc Description: This is a digitally signed message part
Re: [GSoC] A blog about 'git stash' project
On 21 May 2018 01:03:22 GMT+05:30, Paul-Sebastian Ungureanuwrote: > > I actually wrote a >short paragraph about one of them (the one regarding -p option) on the >blog (the first post). > That's interesting. I didn't realise that you wrote about one of the bugs in your blog. I might have missed it, somehow. Anyhow, it happens to me all the time ;-) -- Sivaraam Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: jk/branch-l-0-deprecation (was Re: What's cooking in git.git (May 2018, #02; Thu, 17))
Hi Ævar, On Thursday 17 May 2018 03:18 PM, Ævar Arnfjörð Bjarmason wrote: > I've ended up with that $LESS setting via hackery over the years, so > maybe I'm doing something retarded, minimal test case: > > PAGER=less LESS="--no-init --quit-if-one-screen" git branch -l > > ... > > So I think this is probably OK for most users, if the have very few > branches they'll see it, and then if they use default pager settings > they'll see the stderr output once they quit the pager. I don't know how > common my (mis)configuration is. > I'm not sure this is ok, because I still see the stderr output with your minimal test case even when I have enough branches to not fit in one screen. The stderr output is of course above the pager output (after I quit the pager) and gets hidden out-of display as I stated before. I also get weird 'ESC[m' characters with you minimal test case. I'm not sure what I'm missing. What version of 'less' do you use? Is any other configuration that you didn't mention affecting what your observation? -- Sivaraam QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - John Sonmez Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities in the place I live as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
Re: [GSoC] A blog about 'git stash' project
On Thursday 17 May 2018 02:39 PM, Johannes Schindelin wrote: > I have great empathy for the desire to see these bugs fixed. The > conversion must come first, though, and in the interest of making it > easier on me and other reviewers, I must insist on keeping the conversion > free of any changes, much in the way as we try to avoid evil merges (i.e. > merge commits that introduce changes that were not present in any of their > parents). > Of course, the conversion should be separate from the bug fixes :-) When I mentioned "while porting it to C", I actually meant the "thought process of creating a foundation for `git-stash` in C". I thought hinting at some of the existing and unsolved `git-stash` bugs would allow the person who would be doing the port of `git-stash` to C to consider how to avoid this while implementing the basic foundation. I should have been more explicit about this in my previous mails. -- Sivaraam QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - John Sonmez Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities in the place I live as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
Re: [GSoC] A blog about 'git stash' project
On Thursday 17 May 2018 12:28 PM, Kaartic Sivaraam wrote: > Hi Sebi, > > I thought of pointing you to one of the issues with the current > implementation of 'git stash' which you could probably fix while porting > it to C. > > ... > Forgot to mention about another issue, which I consider to be a bug. I have elaborated about it in the following mailing list email. https://public-inbox.org/git/aa43f1ff-9095-fb4d-43bc-bf8283b7d...@gmail.com/ Unfortunately, it didn't receive any replies. See, if you could do something about it. -- Sivaraam QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - John Sonmez Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities in the place I live as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
Re: [GSoC] A blog about 'git stash' project
Hi Sebi, I thought of pointing you to one of the issues with the current implementation of 'git stash' which you could probably fix while porting it to C. It's about stashing untracked files. You could find more information about it in the following mailing list thread: https://public-inbox.org/git/1505626069.9625.6.ca...@gmail.com/ -- Sivaraam QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - John Sonmez Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities in the place I live as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
jk/branch-l-0-deprecation (was Re: What's cooking in git.git (May 2018, #02; Thu, 17))
On Thursday 17 May 2018 11:31 AM, Junio C Hamano wrote: > * jk/branch-l-0-deprecation (2018-03-26) 3 commits > > ... > > The "-l" option in "git branch -l" is an unfortunate short-hand for > "--create-reflog", but many users, both old and new, somehow expect > it to be something else, perhaps "--list". This step deprecates > the short-hand and warns about the future removal of the it when it > is used. > > Will cook in 'next'. > Perhaps merge to 'master' immediately after 2.18 release? I still have a slight feeling that we shouldn't list the branches for "git branch -l" during the deprecation period. If feel this because i) It would avoid confusions for the users during the deprecation period ii) The warning message seems to add to the confusion: $ git branch -l warning: the '-l' alias for '--create-reflog' is deprecated; warning: it will be removed in a future version of Git * master ... If there are ample branches, the warning message might be hidden out of screen but we shouldn't rely on that, I suppose. -- Sivaraam QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - John Sonmez Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities in the place I live as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
Re: [PATCH v2] mailmap: update brian m. carlson's email address
On Wednesday 09 May 2018 05:49 AM, brian m. carlson wrote: > Correct, it doesn't. In my case, I was using --pretty='%aN <%aE>', > which is how I noticed it in the first place. So, how about updating the commit message to avoid confusions to the incidental future reader? (Or is it just not worth it? ;-]) -- Sivaraam signature.asc Description: OpenPGP digital signature
Re: [RFC PATCH v4 1/3] Teach remote add the --remote-tags option
On Tuesday 01 May 2018 10:29 PM, Wink Saville wrote: > When --remote-tags is passed to `git remote add` the tagopt is set to > --remote-tags and a second fetch line is added so tags are placed in > a separate hierarchy per remote. > I find '--remote' in the option name to be redundant given that it is an option to `git remote add`. I guess '--namespace-tags' would be a better alternative as it seems to convey the meaning more directly to the user. -- Sivaraam QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - Joel Spolsky Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities in the place I live as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
Re: [PATCH 3/8] push tests: add more testing for forced tag pushing
On Monday 30 April 2018 01:50 AM, Ævar Arnfjörð Bjarmason wrote: > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh > index 15c8d5a734..c9a2011915 100755 > --- a/t/t5516-fetch-push.sh > +++ b/t/t5516-fetch-push.sh > @@ -981,7 +981,17 @@ test_expect_success 'push requires --force to update > lightweight tag' ' > git push --force ../child2 Tag && > git tag -f Tag HEAD~ && > test_must_fail git push ../child2 Tag && > - git push --force ../child2 Tag > + git push --force ../child2 Tag && > + git tag -f Tag && > + test_must_fail git push ../child2 "refs/tags/*:refs/tags/*" && > + git push --force ../child2 "refs/tags/*:refs/tags/*" && > + git tag -f Tag HEAD~ && > + git push ../child2 "+refs/tags/*:refs/tags/*" && > + git tag -f Tag && > + git push --no-force ../child2 "+refs/tags/*:refs/tags/*" && > + git tag -f Tag HEAD~ && > + test_must_fail git push ../child2 tag Tag && > + git push --force ../child2 tag Tag As a person who came to know about the "tag " refspec for the first time while seeing this patch, I found it a little hard to parse the following two lines of the test: test_must_fail git push ../child2 tag Tag && git push --force ../child2 tag Tag Maybe some other name than "Tag" for the example would have made it easier for the person reading it. Something like "foo"/"bar" etc. -- Sivaraam QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - Joel Spolsky Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities in the place I live as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
Re: [PATCH 3/8] push tests: add more testing for forced tag pushing
On Tuesday 08 May 2018 08:49 AM, Junio C Hamano wrote: > Junio C Hamanowrites: > >> I couldn't quite get what you meant by "(but not the other way >> around)". Did you mean >> >> $ git push --force ../child2 refs/tags/*:refs/tags/* >> >> should not become non-forcing version because of the (lack of) >> prefix on the refspec does not trump the --force command line >> option? When I was reading the commit message, I had the same doubt about what "(but not the other way around)" actually meant but I assumed it meant that "the `--no-force` in the command line does not override the '+' in the refspec". Maybe the commit message could be updated to clarify this? -- Sivaraam QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - Joel Spolsky Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities in the place I live as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
Re: Weak option parsing in `git submodule`
On Tuesday 08 May 2018 12:35 AM, Stefan Beller wrote: >> The lack of checking for the reason behind why `git add` fails seems to >> be the reason behind that weird message. > > (from the man page) > git submodule [--quiet] add [] [--] [] > > When options are given after or we can count > the arguments and know something is up. (The number of arguments > must be 1 or 2. If it is 3 or above, something fishy is going on), which > I would suggest as a first step. > >> Ways to fix this: >> >> 1. Fix this particular issue by adding a '--' after the '--no-warn- >> embedded-repo' option in the above check. But that would also >> require that we allow other parts of the script to accept weird >> paths such as '--path'. Not so useful/helpful. >> >> 2. Check for the actual return value of `git add` in the check and >> act accordingly. Also, check if there are unnecessary arguments for >> `submodule add`. > > The second part of this suggestion seems to me as the way to go. > Do you want to write a patch? > Incidentally, I was writing a patch to check for the return value of `git add` to fix the particular issue I noted in my initial message. Then I was in a dilemma as to whether this was the right way to do it. So, I thought it would be better to ask before continuing with the patch and hence started this thread. I wasn't counting the arguments to `git submodule add` at that time. Now that I'm ensured that my initial approach is not the worst way to do things and as I'm about to write a patch for this, I'll sum up what I'm about to achieve in the short-term fix patch, for the sake of clarity. 1. Check the return value of `git add ...` and throw an error appropriately. 2. Check the no. of arguments to `submodule add` and throw an error if there are more arguments than there should be. I require a little clarification with this. How should this be done. Does checking whether the number of arguments after is not more than one do the job? Or am I missing something? >> 3. Tighten option parsing in the `git-submodule` script to ensure >> this doesn't happen in the long term and helps users to get more >> relevant error messages. >> >> I find 3 to be a long term solution but not sure if it's worth trying >> as there are efforts towards porting `git submodule` to C. So, I guess >> we should at least do 2 as a short-term solution. > > Yeah, bringing it to C, would be nice as a long term solution instead > of piling up more and more shell features. :) > Hope the day it is brought into C comes soon. > Thanks for such a well written bug report with suggested bug fixes. :) You're welcome :-) -- Sivaraam QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - Joel Spolsky Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities in the place I live as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
Re: main url for linking to git source?
On Monday 07 May 2018 11:45 PM, Stefan Beller wrote: >> I could see arguments both ways, so I thought I'd take a straw poll of >> what people on the list think. > > Junios reply below focuses on the URL passed to git-clone, which > is only found at https://git-scm.com/downloads (?) > > There I would try to mirror Junios list of "public repositories" > https://git-blame.blogspot.com/p/git-public-repositories.html > without officially endorsing one over another. > FWIW, I also seem to support this suggestion as it's not opinionated. > For those links that link to web pages, I am ok with any of the > hosting providers, maybe even taking the one with the prettiest > web page. Maybe we want to reword those sections to rely > more on indirection, e.g. "get the source[link to the source page], > checkout the next branch", without the quick web link to a web page > showing the 'next' tree. Seems to be a nice suggestion to avoid the "main/official" url issue. To add a little more, it might be better replace the "Source code" link with a link to Junio's list of public repositories stated above. Also, it might be better to rename the link to "Public repositories containing the source". -- Sivaraam QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - Joel Spolsky Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities in the place I live as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
Re: [PATCH v2] mailmap: update brian m. carlson's email address
On Tuesday 08 May 2018 07:28 AM, brian m. carlson wrote: > An earlier change, cdb6b5ac (".mailmap: Combine more (name, email) to > individual persons", 2013-08-12), noted that there were two name > spellings and two email addresses and mapped the crustytoothpaste.net > address to the crustytoothpaste.ath.cx address. The latter is an older, > obsolete address, while the former is current, so switch the order of > the addresses so that git log displays the correct address. > Just to be sure, you're meaning the use of `git log --use-mailmap` when you mean `git log` in the log message. Am I right? Or did you mean `git shortlog` ? I'm asking this because I think the `git log` output doesn't consider the mailmap file by default. -- Sivaraam QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - Joel Spolsky Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities in the place I live as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
Re: [PATCH 3/8] push tests: add more testing for forced tag pushing
Hi, On Monday 30 April 2018 01:50 AM, Ævar Arnfjörð Bjarmason wrote: > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh > index 15c8d5a734..c9a2011915 100755 > --- a/t/t5516-fetch-push.sh > +++ b/t/t5516-fetch-push.sh > @@ -981,7 +981,17 @@ test_expect_success 'push requires --force to update > lightweight tag' ' I think the test description has become misleading now that it's testing for 'force pushing' in general and not just the '--force' option. So, a better description is needed. Probably, "force pushing required to update lightweight tag". > git push --force ../child2 Tag && > git tag -f Tag HEAD~ && > test_must_fail git push ../child2 Tag && > - git push --force ../child2 Tag > + git push --force ../child2 Tag && > + git tag -f Tag && > + test_must_fail git push ../child2 "refs/tags/*:refs/tags/*" && > + git push --force ../child2 "refs/tags/*:refs/tags/*" && > + git tag -f Tag HEAD~ && > + git push ../child2 "+refs/tags/*:refs/tags/*" && > + git tag -f Tag && > + git push --no-force ../child2 "+refs/tags/*:refs/tags/*" && This test shouldn't hiding within the tests about force pushing. It seems to warrant a separate test case to clearly note the behavior that the "+" in refspec overrides "--no-force". This would help in easily identifying if this particular behavior is broken or not. -- Sivaraam QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - Joel Spolsky Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities in the place I live as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
Weak option parsing in `git submodule`
I recently faced the consequence of the weak option parsing done by in `git submodule`. Initially tried to add a new submodule using the `git submodule add` sub-command in the following way, $ git submodule add ./foo/bar This cloned the submodule into a new directory named 'bar' in the present directory. This was initially confusing as I expected `git submodule` to use the actual directory itself as it resides inside a sub-directory the main project and thus avoiding the creation of a new clone. Then I came to know that `git submodule` wasn't smart enough yet to identify this, by reading the documentation. Then, after realizing that I would have to specify the path in the command to avoid the redundant clone, I corrected the command without consulting the documentation (thoroughly) to, $ git submodule add ./foo/bar --path ./foo/bar expecting that the path needs to be specified after an argument. This is what triggered the weird consequence of weak option parsing. The output I got for the above command was: The following path is ignored by one of your .gitignore files: --path Use -f if you really want to add it. That really confused me pretty much (being a person who doesn't know much about how `git submodule` works). Instead of complaining about an inexistent option '--path', it considers '--path' to be the argument which seems to be bad. It doesn't even complain about the extra argument. I also dug to find the reason behind this totally weird message. It seems to be a consequence of the following lousy check ($sm_path is the normalized argument): if test -z "$force" && ! git add --dry-run --ignore-missing --no-warn-embedded-repo "$sm_path" > /dev/null 2>&1 then eval_gettextln "The following path is ignored by one of your .gitignore files: \$sm_path Use -f if you really want to add it." >&2 exit 1 fi The lack of checking for the reason behind why `git add` fails seems to be the reason behind that weird message. Ways to fix this: 1. Fix this particular issue by adding a '--' after the '--no-warn- embedded-repo' option in the above check. But that would also require that we allow other parts of the script to accept weird paths such as '--path'. Not so useful/helpful. 2. Check for the actual return value of `git add` in the check and act accordingly. Also, check if there are unnecessary arguments for `submodule add`. 3. Tighten option parsing in the `git-submodule` script to ensure this doesn't happen in the long term and helps users to get more relevant error messages. I find 3 to be a long term solution but not sure if it's worth trying as there are efforts towards porting `git submodule` to C. So, I guess we should at least do 2 as a short-term solution. Thoughts?? -- Sivaraam QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - Joel Spolsky Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance!
Re: [GSoC] A blog about 'git stash' project
Hi Sebi, On Friday 04 May 2018 03:18 AM, Paul-Sebastian Ungureanu wrote: > Hello everybody, > > The community bonding period started. It is well known that for a > greater rate of success, it is recommended to send weekly reports > regarding project status. But, what would be a good form for such a > report? I, for one, think that starting a blog is one of the best > options because all the content will be stored in one place. Without > further ado, I would like you to present my blog [1]. > > Any feedback is greatly appreciated! Thank you! > The blog looks pretty well written. I also read your proposal. It also seems to be pretty much well written. I like the way you explain things. Particularly, you seem to be explaining the problem and the way you're about to approach it well. The plan seems pretty good. I just thought of suggesting one thing which might possibly be redundant. I think you're aware of the fact that the Git project has Travis-CI builds enabled[1] which you could take advantage of to ensure your changes pass in various text environments. If you're interested in testing your changes (which I suspect you are), you might also be interested in 'git-test'[2], a tool built by Michael Haggerty. Unlike the Travis-CI tests which test only the tip of the change, 'git-test' would help you ensure that every single commit you make doesn't break the test suite (which is both a nice thing and is expected here). Sorry for the off-topic info about the tests in this mail :-) Hope you're able to achieve your goal as planned and have a great time during this summer of code! References: [1]: https://github.com/git/git/blob/master/Documentation/SubmittingPatches#L70 [2]: https://github.com/mhagger/git-test Regards, Kaartic QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - Joel Spolsky KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
Re: [PATCH] git: add -N as a short option for --no-pager
On Wednesday 25 April 2018 11:55 AM, Johannes Sixt wrote: > > I considered -P, but thought that it would better be reserved for > something related to "paths". We have --{html,man,info}-path, which > would be served better with -[HMI]. That leaves --exec-path, which we > would probably abbreviate as -x or -X. So, -P is perhaps not that bad > after all. > It might be a good choice but I don't think it's not the best choice anyway because '-P' doesn't seem to communicate 'no pager' well. > Perhaps --no-pager means also "unpaginated" (-u, -U), "linear" (-l, -L), > "streamed" (-s, -S). Other ideas, anyone? > Considering the other alternatives you suggested (as I couldn't come up with a better "single letter" option ;-) ), I think '-s' might be a nice choice but would possibly need some explanation in the documentation to the wondering users. In the end, lacking a better one letter option, I think '-P' would be a better choice after all. It would, possibly, be easily understood as a negation of pagination (-p). So, I guess it would be better to go with '-P' than '-N'. -- Sivaraam QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - Joel Spolsky KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
Re: [PATCH] git: add -N as a short option for --no-pager
On Thursday 26 April 2018 01:01 AM, Johannes Sixt wrote: > > Right. But I have LESS=RS because I do *not* want it to remain on screen > by default. > In that case how about updating the commit message to be more clear about it. How about, It is possible to configure 'less', the pager, to use an alternate screen to show the content. When it is closed in this case, it switches back to the original screen, and all content is gone. -- Sivaraam QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - Joel Spolsky KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
Re: Draft of Git Rev News edition 38
Hi, On Tuesday 17 April 2018 03:56 AM, Christian Couder wrote: > Hi, > > On Mon, Apr 16, 2018 at 5:07 PM, Kaartic Sivaraam > <kaartic.sivar...@gmail.com> wrote: >> >> That said, I read the draft and found it good except for two minor issues, > > Thanks for your comments! > You're welcome! I'm sorry to say that I read only part of the draft when I sent my previous email though I accidentally didn't mention it explicitly. Now that I have read the draft completely I find a few typos in the "Developer Spotlight: Jiang Xin" section: 1. "... because I feel it is hard to track changes of GitHub UI and the book will become obsolte very quickly." obsolte -> obsolete 2. "We also developped ..." developped -> developed On seeing the section "Light reading" to be empty, I thought I could suggest something. I'm not sure whether you take Stack Overflow answers for a light reading but I found the following answer to be interesting, https://stackoverflow.com/a/6521223/5614968 That's all. -- Kaartic QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - Joel Spolsky signature.asc Description: OpenPGP digital signature
Re: Draft of Git Rev News edition 38
Hi, On Monday 16 April 2018 08:33 PM, Sergey Organov wrote: > Christian Couderwrites: >> Here "the above article" means the Jake's "branch -l: print useful >> info whilst rebasing a non-local branch" article above the current >> article. Just a little correction. I suppose Chris actually meant the "rebase -i: offer to recreate merge commits" article written by Jake and not the "branch -l: print useful info whilst rebasing a non-local branch" article. That said, I read the draft and found it good except for two minor issues, 1. I see the following sentence in the "Rebasing merges: a jorney to the ultimate solution (Road Clear) (written by Jacob Keller)" article "A few examples were tried, but it was proven that the original concept did not work, as dropped commits could end up being replaid into the merge commits, turning them into "evil" merges." I'm not sure if 'replaid' is proper English assuming the past tense of replay was intended there (which I think is 'replayed'). 2. I see a minor Markdown syntax issue in the "branch -l: print useful info whilst rebasing a non-local branch" article. ... reworked his original patch to improve `git branch --list̀ Specifically, in the '--list̀' part. I guess it should be "--list`". -- Kaartic QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - Joel Spolsky signature.asc Description: OpenPGP digital signature
[PATCH v3 2/2] t3200: verify "branch --list" sanity when rebasing from detached HEAD
From: Eric Sunshine <sunsh...@sunshineco.com> "git branch --list" shows an in-progress rebase as: * (no branch, rebasing ) master ... However, if the rebase is started from a detached HEAD, then there is no , and it would attempt to print a NULL pointer. The previous commit fixed this problem, so add a test to verify that the output is sane in this situation. Signed-off-by: Eric Sunshine <sunsh...@sunshineco.com> Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- t/t3200-branch.sh | 24 1 file changed, 24 insertions(+) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 503a88d02..89fff3fa9 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -6,6 +6,7 @@ test_description='git branch assorted tests' . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-rebase.sh test_expect_success 'prepare a trivial repository' ' echo Hello >A && @@ -1246,6 +1247,29 @@ test_expect_success '--merged is incompatible with --no-merged' ' test_must_fail git branch --merged HEAD --no-merged HEAD ' +test_expect_success '--list during rebase' ' + test_when_finished "reset_rebase" && + git checkout master && + FAKE_LINES="1 edit 2" && + export FAKE_LINES && + set_fake_editor && + git rebase -i HEAD~2 && + git branch --list >actual && + test_i18ngrep "rebasing master" actual +' + +test_expect_success '--list during rebase from detached HEAD' ' + test_when_finished "reset_rebase && git checkout master" && + git checkout master^0 && + oid=$(git rev-parse --short HEAD) && + FAKE_LINES="1 edit 2" && + export FAKE_LINES && + set_fake_editor && + git rebase -i HEAD~2 && + git branch --list >actual && + test_i18ngrep "rebasing detached HEAD $oid" actual +' + test_expect_success 'tracking with unexpected .fetch refspec' ' rm -rf a b c d && git init a && -- 2.17.0.484.g0c8726318
Re: [PATCH v2 2/2] t3200: verify "branch --list" sanity when rebasing from detached HEAD
On Tuesday 03 April 2018 01:30 PM, Eric Sunshine wrote: >> Note that the "detached HEAD" test case might actually fail in some cases >> as the actual output of "git branch --list" might contain remote branch >> names which is not considered by the test case as it is rare to happen >> in the test environment. > > This paragraph was not in the original patch[1]. I _think_ what you > are saying (which took a while to decipher) is that if a command such > as "git checkout origin/next" ever gets inserted into the script > before the test, the test will be fooled since "git branch --list" > will show "detached HEAD origin/next" rather than "detached HEAD > d3adb33f", the latter of which is what the test is expecting. > Yeah, you're right. To know the reason for the unclear paragraph, see below. > Unfortunately, this paragraph makes it sound as if the test can fail > randomly (which, I believe, is not the case), and nobody would want a > test added which is unreliable, thus this paragraph is not helping to > sell this patch (in fact, it's actively hurting it). Ideally, the test > should be entirely deterministic so that it can't be fooled like this. > Rather than including this (harmful) paragraph in the commit message, > let's ensure that the test is deterministic (see below). > Sorry for the harmful and not so clear paragraph! I actually kept that paragraph there to **remind me** that I have to fix the issue which it describes before sending out the patch but I somehow forgot about it after I added it initially :-( >> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh >> @@ -1246,6 +1247,29 @@ test_expect_success '--merged is incompatible with >> --no-merged' ' >> +test_expect_success '--list during rebase from detached HEAD' ' >> + test_when_finished "reset_rebase && git checkout master" && >> + git checkout HEAD^0 && > > This is the line which I think is causing concern for you. If someone > inserted a new test before this one which invoked "git checkout > origin/next" (or something), then even after "git checkout HEAD^0", > "git branch --list" would still report the unexpected "detached HEAD > origin/next". Let's fix this, and make the test deterministic, by > doing this instead: > > git checkout master^0 && > Nice idea, will re-send a v3 with this fix and the harmful paragraph removed. Thanks, Kaartic signature.asc Description: OpenPGP digital signature
[PATCH v2 0/2] branch --list: print useful info whilst interactive rebasing a detached HEAD
Ok, I seem to have forgotten the cover letter. So, here's one. The changes from v1 are as follows: * Changes to the commit message of 1/2 to fix some errors * Code changes to 1/2 to address the comments from v1 * Patch 2/2 is new. It's adds tests for the issue that 1/2 tries to fix. It's written by Eric with a little fix by me to make it work with GETTEXT_POISON. An interdiff for 1/2: diff --git a/ref-filter.c b/ref-filter.c index a4c917c96..db2baedfe 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1309,18 +1309,14 @@ char *get_head_description(void) memset(, 0, sizeof(state)); wt_status_get_state(, 1); if (state.rebase_in_progress || - state.rebase_interactive_in_progress) - { - const char *rebasing = NULL; - if (state.branch != NULL) - rebasing = state.branch; + state.rebase_interactive_in_progress) { + if (state.branch) + strbuf_addf(, _("(no branch, rebasing %s)"), + state.branch); else - rebasing = state.detached_from; - - strbuf_addf(, _("(no branch, rebasing %s)"), - rebasing); - } - else if (state.bisect_in_progress) + strbuf_addf(, _("(no branch, rebasing detached HEAD %s)"), + state.detached_from); + } else if (state.bisect_in_progress) strbuf_addf(, _("(no branch, bisect started on %s)"), state.branch); else if (state.detached_from) { Eric Sunshine (1): t3200: verify "branch --list" sanity when rebasing from detached HEAD Kaartic Sivaraam (1): branch --list: print useful info whilst interactive rebasing a detached HEAD ref-filter.c | 12 t/t3200-branch.sh | 24 2 files changed, 32 insertions(+), 4 deletions(-) signature.asc Description: OpenPGP digital signature
[PATCH v2 1/2] branch --list: print useful info whilst interactive rebasing a detached HEAD
When rebasing interactively (rebase -i), "git branch --list" prints a line indicating the current branch being rebased. This works well when the interactive rebase is initiated when a local branch is checked out. This doesn't play well when the rebase is initiated on a detached HEAD. When "git branch --list" tries to print information related to the interactive rebase in this case it tries to print the name of a branch using an uninitialized variable and thus tries to print a "null pointer string". As a consequence, it does not provide useful information while also inducing undefined behaviour. So, print the point from which the rebase was started when interactive rebasing a detached HEAD. Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- ref-filter.c | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index f9e25aea7..db2baedfe 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1309,10 +1309,14 @@ char *get_head_description(void) memset(, 0, sizeof(state)); wt_status_get_state(, 1); if (state.rebase_in_progress || - state.rebase_interactive_in_progress) - strbuf_addf(, _("(no branch, rebasing %s)"), - state.branch); - else if (state.bisect_in_progress) + state.rebase_interactive_in_progress) { + if (state.branch) + strbuf_addf(, _("(no branch, rebasing %s)"), + state.branch); + else + strbuf_addf(, _("(no branch, rebasing detached HEAD %s)"), + state.detached_from); + } else if (state.bisect_in_progress) strbuf_addf(, _("(no branch, bisect started on %s)"), state.branch); else if (state.detached_from) { -- 2.17.0.rc0.231.g781580f06
[PATCH v2 2/2] t3200: verify "branch --list" sanity when rebasing from detached HEAD
From: Eric Sunshine <sunsh...@sunshineco.com> "git branch --list" shows an in-progress rebase as: * (no branch, rebasing ) master ... However, if the rebase is started from a detached HEAD, then there is no , and it would attempt to print a NULL pointer. The previous commit fixed this problem, so add a test to verify that the output is sane in this situation. Note that the "detached HEAD" test case might actually fail in some cases as the actual output of "git branch --list" might contain remote branch names which is not considered by the test case as it is rare to happen in the test environment. Signed-off-by: Eric Sunshine <sunsh...@sunshineco.com> Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- t/t3200-branch.sh | 24 1 file changed, 24 insertions(+) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 503a88d02..738b5eb22 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -6,6 +6,7 @@ test_description='git branch assorted tests' . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-rebase.sh test_expect_success 'prepare a trivial repository' ' echo Hello >A && @@ -1246,6 +1247,29 @@ test_expect_success '--merged is incompatible with --no-merged' ' test_must_fail git branch --merged HEAD --no-merged HEAD ' +test_expect_success '--list during rebase' ' + test_when_finished "reset_rebase" && + git checkout master && + FAKE_LINES="1 edit 2" && + export FAKE_LINES && + set_fake_editor && + git rebase -i HEAD~2 && + git branch --list >actual && + test_i18ngrep "rebasing master" actual +' + +test_expect_success '--list during rebase from detached HEAD' ' + test_when_finished "reset_rebase && git checkout master" && + git checkout HEAD^0 && + oid=$(git rev-parse --short HEAD) && + FAKE_LINES="1 edit 2" && + export FAKE_LINES && + set_fake_editor && + git rebase -i HEAD~2 && + git branch --list >actual && + test_i18ngrep "rebasing detached HEAD $oid" actual +' + test_expect_success 'tracking with unexpected .fetch refspec' ' rm -rf a b c d && git init a && -- 2.17.0.rc0.231.g781580f06
Re: [PATCH] branch -l: print useful info whilst rebasing a non-local branch
On Sunday 25 March 2018 11:18 AM, Eric Sunshine wrote: > On Sun, Mar 25, 2018 at 09:11:34AM +0530, Kaartic Sivaraam wrote: >> On Sunday 25 March 2018 07:04 AM, Eric Sunshine wrote: >>> Can we have a couple new tests: one checking "git branch --list" for >>> the typical case (when rebasing off a named branch) and one checking >>> when rebasing from a detached HEAD? >> >> Sure, but I guess it would take some time for me to add the tests. I'll >> send a v2 with the suggested changes. > > A couple more comments: > > * Please run the commit message through a spell checker; it contains > several typographical errors. > Thanks for motivating me to search for a spell checker. I have now discovered the spell check feature (:set spell) in Vim! > * I wonder if it makes sense to give slightly different output in the > detached HEAD case. Normal output is: > > (no branch, rebasing ) > > and, with your change, detached HEAD output is: > > (no branch, rebasing d3adb33f) > > which is okay, but perhaps it could be better; for instance: > > (no branch, rebasing detached HEAD d3adb33f) > I just recently discovered that the variable used to print information related to detached HEAD (state.detached_from) might also contain remote branch names (origin/master, etc.) other than commit hashes. So, it might make sense to distinguish detached HEAD. > Anyhow, I wrote the tests for you. When you re-roll, you can make the > following patch 2/2 and your fix 1/2. Thanks a lot! > (If you go with the above idea > of using a slightly different wording for the detached HEAD case, then > you'll need to adjust the 'grep' slightly in the second test.) > > --- >8 --- > From: Eric Sunshine <sunsh...@sunshineco.com> > Date: Sun, 25 Mar 2018 01:29:58 -0400 > Subject: [PATCH] t3200: verify "branch --list" sanity when rebasing from > detached HEAD > > "git branch --list" shows an in-progress rebase as: > > * (no branch, rebasing ) > master > ... > > However, if the rebase is started from a detached HEAD, then there is no > , and it would attempt to print a NULL pointer. The previous > commit fixed this problem, so add a test to verify that the output is > sane in this situation. > > Signed-off-by: Eric Sunshine <sunsh...@sunshineco.com> > --- > t/t3200-branch.sh | 24 > 1 file changed, 24 insertions(+) > > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh > index 6c0b7ea4ad..d1f80c80ab 100755 > --- a/t/t3200-branch.sh > +++ b/t/t3200-branch.sh > @@ -6,6 +6,7 @@ > test_description='git branch assorted tests' > > . ./test-lib.sh > +. "$TEST_DIRECTORY"/lib-rebase.sh > > test_expect_success 'prepare a trivial repository' ' > echo Hello >A && > @@ -1246,6 +1247,29 @@ test_expect_success '--merged is incompatible with > --no-merged' ' > test_must_fail git branch --merged HEAD --no-merged HEAD > ' > > +test_expect_success '--list during rebase' ' > + test_when_finished "reset_rebase" && > + git checkout master && > + FAKE_LINES="1 edit 2" && > + export FAKE_LINES && > + set_fake_editor && > + git rebase -i HEAD~2 && > + git branch --list >actual && > + grep "rebasing master" actual > +' > + > +test_expect_success '--list during rebase from detached HEAD' ' > + test_when_finished "reset_rebase && git checkout master" && > + git checkout HEAD^0 && > + oid=$(git rev-parse --short HEAD) && > + FAKE_LINES="1 edit 2" && > + export FAKE_LINES && > + set_fake_editor && > + git rebase -i HEAD~2 && > + git branch --list >actual && > + grep "rebasing $oid" actual > +' > + > test_expect_success 'tracking with unexpected .fetch refspec' ' > rm -rf a b c d && > git init a && > -- Kaartic signature.asc Description: OpenPGP digital signature
Re: [PATCH] branch -l: print useful info whilst rebasing a non-local branch
On Sunday 25 March 2018 10:03 AM, Jeff King wrote: > ... > but I'd prefer to avoid those kinds of magic rules if we can. They're > very hard to explain to the user, and can be quite baffling when they go > wrong. > I fell the same too. > IMHO we should do one of: > > 1. Nothing. ;) > > 2. Complain about "-l" in list mode to help educate users about the > current craziness. > > 3. Drop "-l" (probably with a deprecation period); it seems unlikely > to me that anybody uses it for branch creation, and this would at > least reduce the confusion (then it would just be "so why don't we > have -l" instead of "why is -l not what I expect"). > > 4. Repurpose "-l" as a shortcut for --list (also after a deprecation > period). This is slightly more dangerous in that it may confuse > people using multiple versions of Git that cross the deprecation > line. But that's kind of what the deprecation period is for... > I think we should do 2 as a short term fix for sure. For the long term, I would prefer 4 as I think most users would expect "-l" to be a shortcut for "--list" particularly given the current situation that "git branch -l" lists all the branch names. That said, I would not mind considering 3 if 4 has more bad consequences than the good it does (but I heavily doubt it ;-) ). I don't consider 1 to be an option ;-) -- Kaartic signature.asc Description: OpenPGP digital signature
Re: [PATCH] branch -l: print useful info whilst rebasing a non-local branch
On Sunday 25 March 2018 07:04 AM, Eric Sunshine wrote: > On Sat, Mar 24, 2018 at 2:38 PM, Kaartic Sivaraam > <kaartic.sivar...@gmail.com> wrote: >> When rebasing interacitvely (rebase -i), "git branch -l" prints a line > > The "git branch -l" threw me since "-l" is short for --create-reflog. > I'm guessing you meant "git branch --list". > That's surprising, I just tried "git branch -l" on a repository and I did get a list of branch names. Is this a consequence of some option parsing weirdness ?! To be honest, I actually assumed "-l" to be a shorthand for "--list" and didn't check with it in the documentation; which I should have. Sorry, for that. I still wonder why "git branch -l" prints a list of branch names when it is not a shorthand for "--list" ? (BTW, I'm also surprised by the fact that "-l" is not act shorthand for "--list"!) Regardless, I'll update the commit message to use "--list" in place of "-l". >> indicating the current branch being rebased. This works well when the >> interactive rebase was intiated when a local branch is checked out. >> >> This doesn't play well when the rebase was initiated on a remote >> branch or an arbitrary commit that is not pointed to by a local >> branch. > > A shorter way of saying "arbitrary commit ... not pointed at by local > branch" would be "detached HEAD". > Thanks. I was actually searching for this word. It didn't strike when I wrote the commit message, yesterday. > You could collapse the whole thing back down to: > > strbuf_addf(, _("(no branch, rebasing %s)"), > state.branch ? state.branch : state.detached_from); > > which means you don't need the 'rebasing' variable or the braces. > Nice point. > Can we have a couple new tests: one checking "git branch --list" for > the typical case (when rebasing off a named branch) and one checking > when rebasing from a detached HEAD? > Sure, but I guess it would take some time for me to add the tests. I'll send a v2 with the suggested changes. -- Kaartic signature.asc Description: OpenPGP digital signature
[PATCH] branch -l: print useful info whilst rebasing a non-local branch
When rebasing interacitvely (rebase -i), "git branch -l" prints a line indicating the current branch being rebased. This works well when the interactive rebase was intiated when a local branch is checked out. This doesn't play well when the rebase was initiated on a remote branch or an arbitrary commit that is not pointed to by a local branch. In this case "git branch -l" tries to print the name of a branch using an unintialized variable and thus tries to print a "null pointer string". As a consequence, it does not provide useful information while also inducing undefined behaviour. So, print the commit from which the rebase started when interactive rebasing a non-local branch. Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- ref-filter.c | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/ref-filter.c b/ref-filter.c index f9e25aea7..a4c917c96 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1310,8 +1310,16 @@ char *get_head_description(void) wt_status_get_state(, 1); if (state.rebase_in_progress || state.rebase_interactive_in_progress) + { + const char *rebasing = NULL; + if (state.branch != NULL) + rebasing = state.branch; + else + rebasing = state.detached_from; + strbuf_addf(, _("(no branch, rebasing %s)"), - state.branch); + rebasing); + } else if (state.bisect_in_progress) strbuf_addf(, _("(no branch, bisect started on %s)"), state.branch); -- 2.17.0.rc0.231.g781580f06
Re: [PATCH v4 2/3] builtin/branch: give more useful error messages when renaming
On Friday 16 March 2018 02:03 AM, Junio C Hamano wrote: > Quite honestly, I am not sure if this amount of new code that > results in sentence lego is really worth it. Speaking specifically about the new code for the sentence lego: I currently lack knowledge of a better way to achieve the same outcome the new code does. Let me know if there is a better way. > Is it so wrong for > "branch -m tset master" to complain that master already exists so no > branch can be renamed to it? > Speaking in general about the patch itself: though I still find the fact that "the error about an inexistent source branch seconds the error about an existing destination branch" to be a little unintuitive, I actually went on to reboot this after a long time as this also seems to bring consistency in the error messages related to moving a branch. It seems that the commit message requires an update as it currently seems to be misleading as it currently doesn't specify the motivation completely. That said, I won't be against dropping the patch if it seems to be adding less value at the cost of more code. -- Kaartic signature.asc Description: OpenPGP digital signature
Re: [PATCH v4 1/3] branch: introduce dont_fail parameter for branchname validation
On Friday 16 March 2018 01:57 AM, Junio C Hamano wrote: > Kaartic Sivaraam <kaartic.sivar...@gmail.com> writes: > > So... I am not finding dont_fail that was mentioned on the title > anywhere else in the patch. Such lack of attention to detail is > a bit off-putting. > I'm absolutely sorry x-< I should have forgotten to update the commit subject during one of the old rebases in which I renamed the parameter from 'dont_fail' to 'gently'. I shouldn't have assumed that the commit messages of the old series held in the reboot too. I should have re-read them "completely" and double checked it. :-( >> +enum branch_validation_result { >> +/* Flags that convey there are fatal errors */ >> +VALIDATION_FATAL_BRANCH_EXISTS_NO_FORCE = -3, >> +VALIDATION_FATAL_CANNOT_FORCE_UPDATE_CURRENT_BRANCH, >> +VALIDATION_FATAL_INVALID_BRANCH_NAME, >> +/* Flags that convey there are no fatal errors */ >> +VALIDATION_PASS_BRANCH_DOESNT_EXIST = 0, >> +VALIDATION_PASS_BRANCH_EXISTS = 1, >> +VALIDATION_WARN_BRANCH_EXISTS = 2 >> +}; > > where adding new error types will force us to touch _two_ lines > (i.e. either you add a new error before NO_FORCE with value -4 and > then remove the "= -3" from NO_FORCE, or you add a new error after > INVALID, and update NO_FORCE to -4), which can easily be screwed up > by a careless developer. The current code is not wrong per-se, but > I wonder if it can be made less error prone. > At the top of my head I could think of 2 ways to get around this, - Assigning the actual value to every single entry in the enum. This should solve the issue as a any new entry that would be added is expected to go "with a value". The compiler would warn you in the case of duplicate values. The drawback is: it might be a little restrictive and a little ugly. It would also likely cause maintenance issues if the number of values in the enum get bigger. (Of course this doesn't hold if, the careless programmer shatters "consistency" and adds an entry without a value to the enum and that change gets merged into the codebase ;-) ) - Using non-negative values for both errors and non-errors. This might make it hard to distinguish errors from non-errors but this would avoid errors completely without much issues, otherwise. I might prefer the former as I find the possibility of the requirement to distinguish the errors from non-errors to be high when compared with the possibility of the requirement to add more new entries to the enum. Any other ideas/suggestions ? -- Kaartic QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - Joel Spolsky signature.asc Description: OpenPGP digital signature
Re: Bug, git rebase -i does not abort correctly
On Friday 26 January 2018 02:54 PM, Duy Nguyen wrote: > > Sort of. It smells bad design to me when a mistake can easily become a > feature. But with your help, I think I should be able to disable this > feature on my local build. Thanks. > In case you're still facing this issue, it just struck me recently that you could have a different alias for 'git rebase -i --onto'. In which case you could possibly avoid falling prey to the syntax issue ;-) Something like, $ git config alias.rio 'rebase -i --onto' -- Kaartic QUOTE --- “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - Joel Spolsky signature.asc Description: OpenPGP digital signature
[PATCH v4 0/3] give more useful error messages while renaming branch (reboot)
It's been a long time since the v3 of the patch. So, it's worth restating the reason behind this patch. >From v1 of this patch, In builtin/branch, the error messages weren't handled directly by the branch renaming function and was left to the other function. Though this avoids redundancy this gave unclear error messages in some cases. So, make builtin/branch give more useful error messages. Changes since v3: - Handled more error related to old branch name. - Incorporated changes suggested in v3 which include using ';' as a sentence connector instead 'and'. - Error messages use the interpreted branch names (without the (refs/heads/ part). The unrelated cleanup patches which were in the previous versions have since been submitted as a separate series and have been merged into the codebase. The first two patches are related to the topic of this patch. The 3rd one is a little typo fix that I noticed on the way. This patch was based off 'master' and has been rebased to incorporate the new changes to 'master'. So, it generally should apply cleanly on 'master'. Let me know if it doesn't. The sample input/output cases for this patch are as follows, $ git branch * master foo bar Before patch, # Case 1: Trying to rename non-existent branch $ git branch -m hypothet no_such_branch error: refname refs/heads/hypothet not found fatal: Branch rename failed # Case 2: Trying to rename non-existent branch to an existing one $ git branch -m hypothet master fatal: A branch named 'master' already exists. # Case 3: Trying to force update current branch $ git branch -M foo master fatal: Cannot force update the current branch. # Case 4: Trying to force rename an in-existent branch with an invalid name $ git branch -M hypothet ?123 fatal: '?123' is not a valid branch name. After patch, # Case 1: Trying to rename non-existent branch $ git branch -m hypothet no_such_branch fatal: branch 'hypothet' doesn't exist # Case 2: Trying to rename non-existent branch to an existing one $ git branch -m hypothet master fatal: branch 'hypothet' doesn't exist; branch 'master' already exists # Case 3: Trying to force update current branch $ git branch -M foo master fatal: cannot force update the current branch # Case 4: Trying to force rename an in-existent branch with an invalid name $ git branch -M hypothet ?123 fatal: branch 'hypothet' doesn't exist; new branch name '?123' is invalid Note: Thanks to the strbuf API that made it possible to easily construct the composite error message strings! Kaartic Sivaraam (3): branch: introduce dont_fail parameter for branchname validation builtin/branch: give more useful error messages when renaming t/t3200: fix a typo in a test description branch.c | 59 +--- branch.h | 61 - builtin/branch.c | 111 ++--- builtin/checkout.c | 5 +- t/t3200-branch.sh | 2 +- 5 files changed, 181 insertions(+), 57 deletions(-) -- 2.16.1.291.g4437f3f13
[PATCH v4 1/3] branch: introduce dont_fail parameter for branchname validation
This parameter allows the branchname validation functions to optionally return a flag specifying the reason for failure, when requested. This allows the caller to know why it was about to die. This allows more useful error messages to be given to the user when trying to rename a branch. The flags are specified in the form of an enum and values for success flags have been assigned explicitly to clearly express that certain callers rely on those values and they cannot be arbitrary. Only the logic has been added but no caller has been made to use it, yet. So, no functional changes. Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- branch.c | 59 branch.h | 61 +- builtin/branch.c | 4 +-- builtin/checkout.c | 5 ++-- 4 files changed, 88 insertions(+), 41 deletions(-) diff --git a/branch.c b/branch.c index 2672054f0..0eeb8403e 100644 --- a/branch.c +++ b/branch.c @@ -178,41 +178,48 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name) return 0; } -/* - * Check if 'name' can be a valid name for a branch; die otherwise. - * Return 1 if the named branch already exists; return 0 otherwise. - * Fill ref with the full refname for the branch. - */ -int validate_branchname(const char *name, struct strbuf *ref) +enum branch_validation_result validate_branchname(const char *name, struct strbuf *ref, unsigned gently) { - if (strbuf_check_branch_ref(ref, name)) - die(_("'%s' is not a valid branch name."), name); + if (strbuf_check_branch_ref(ref, name)) { + if (gently) + return VALIDATION_FATAL_INVALID_BRANCH_NAME; + else + die(_("'%s' is not a valid branch name."), name); + } - return ref_exists(ref->buf); + return ref_exists(ref->buf) ? VALIDATION_PASS_BRANCH_EXISTS : VALIDATION_PASS_BRANCH_DOESNT_EXIST; } -/* - * Check if a branch 'name' can be created as a new branch; die otherwise. - * 'force' can be used when it is OK for the named branch already exists. - * Return 1 if the named branch already exists; return 0 otherwise. - * Fill ref with the full refname for the branch. - */ -int validate_new_branchname(const char *name, struct strbuf *ref, int force) +enum branch_validation_result validate_new_branchname(const char *name, struct strbuf *ref, int force, unsigned gently) { const char *head; - if (!validate_branchname(name, ref)) - return 0; + if (gently) { + enum branch_validation_result res = validate_branchname(name, ref, 1); + if (res == VALIDATION_FATAL_INVALID_BRANCH_NAME || res == VALIDATION_PASS_BRANCH_DOESNT_EXIST) + return res; + } else { + if (validate_branchname(name, ref, 0) == VALIDATION_PASS_BRANCH_DOESNT_EXIST) + return VALIDATION_PASS_BRANCH_DOESNT_EXIST; + } - if (!force) - die(_("A branch named '%s' already exists."), - ref->buf + strlen("refs/heads/")); + if (!force) { + if (gently) + return VALIDATION_FATAL_BRANCH_EXISTS_NO_FORCE; + else + die(_("A branch named '%s' already exists."), + ref->buf + strlen("refs/heads/")); + } head = resolve_ref_unsafe("HEAD", 0, NULL, NULL); - if (!is_bare_repository() && head && !strcmp(head, ref->buf)) - die(_("Cannot force update the current branch.")); + if (!is_bare_repository() && head && !strcmp(head, ref->buf)) { + if (gently) + return VALIDATION_FATAL_CANNOT_FORCE_UPDATE_CURRENT_BRANCH; + else + die(_("Cannot force update the current branch.")); + } - return 1; + return VALIDATION_WARN_BRANCH_EXISTS; } static int check_tracking_branch(struct remote *remote, void *cb_data) @@ -259,8 +266,8 @@ void create_branch(const char *name, const char *start_name, explicit_tracking = 1; if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok) - ? validate_branchname(name, ) - : validate_new_branchname(name, , force)) { + ? validate_branchname(name, , 0) + : validate_new_branchname(name, , force, 0)) { if (!force) dont_change_ref = 1; else diff --git a/branch.h b/branch.h index 473d0a93e..ee5f1c0e7 100644 --- a/branch.h +++ b/branch.h @@ -28,20 +28,59 @@ void create_branch(const char *name, const char *start_name, int force, int clobber_head_ok, int reflog, int qu
[PATCH v4 2/3] builtin/branch: give more useful error messages when renaming
When trying to rename an "inexistent" branch name to a branch name that "already exists" the rename failed stating that the new branch name exists rather than stating that the branch trying to be renamed doesn't exist. $ git branch -m tset master fatal: A branch named 'master' already exists. It's conventional to report that 'tset' doesn't exist rather than reporting that 'master' exists, the same way the 'mv' command does. (hypothetical) $ git branch -m tset master fatal: branch 'tset' doesn't exist. That has the problem that the error about an existing branch is shown only after the user corrects the error about inexistent branch. $ git branch -m test master fatal: A branch named 'master' already exists. This isn't useful either because the user would have corrected this error in a single go if he had been told this alongside the first error. So, give more useful error messages by giving errors about old branch name and new branch name at the same time. This is possible as the branch name validation functions now return the reason they were about to die, when requested. $ git branch -m tset master fatal: branch 'tset' doesn't exist; branch 'master' already exists Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- builtin/branch.c | 111 +++ 1 file changed, 94 insertions(+), 17 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index 5412aa78f..ab78a7f45 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -55,6 +55,13 @@ enum color_branch { BRANCH_COLOR_UPSTREAM = 5 }; +enum old_branch_validation_result { + VALIDATION_1_FATAL_OLD_BRANCH_DOESNT_EXIST = -2, + VALIDATION_1_FATAL_INVALID_OLD_BRANCH_NAME = -1, + VALIDATION_1_PASS_OLD_BRANCH_EXISTS = 0, + VALIDATION_1_WARN_BAD_OLD_BRANCH_NAME = 1 +}; + static struct string_list output = STRING_LIST_INIT_DUP; static unsigned int colopts; @@ -458,13 +465,86 @@ static void reject_rebase_or_bisect_branch(const char *target) free_worktrees(worktrees); } +static void get_error_msg(struct strbuf* error_msg, + const char* oldname, enum old_branch_validation_result old_branch_name_res, + const char* newname, enum branch_validation_result new_branch_name_res) +{ + const char* connector_string = "; "; + unsigned append_connector = 0; + + switch (old_branch_name_res) { + case VALIDATION_1_FATAL_INVALID_OLD_BRANCH_NAME: + strbuf_addf(error_msg, + _("old branch name '%s' is invalid"), oldname); + append_connector = 1; + break; + case VALIDATION_1_FATAL_OLD_BRANCH_DOESNT_EXIST: + strbuf_addf(error_msg, + _("branch '%s' doesn't exist"), oldname); + append_connector = 1; + break; + + /* not necessary to handle nonfatal cases */ + case VALIDATION_1_PASS_OLD_BRANCH_EXISTS: + case VALIDATION_1_WARN_BAD_OLD_BRANCH_NAME: + break; + } + + switch (new_branch_name_res) { + case VALIDATION_FATAL_BRANCH_EXISTS_NO_FORCE: + strbuf_addf(error_msg, "%s", + (append_connector) ? connector_string : ""); + strbuf_addf(error_msg, + _("branch '%s' already exists"), newname); + break; + case VALIDATION_FATAL_CANNOT_FORCE_UPDATE_CURRENT_BRANCH: + strbuf_addf(error_msg, "%s", + (append_connector) ? connector_string : ""); + strbuf_addstr(error_msg, + _("cannot force update the current branch")); + break; + case VALIDATION_FATAL_INVALID_BRANCH_NAME: + strbuf_addf(error_msg, "%s", + (append_connector) ? connector_string : ""); + strbuf_addf(error_msg, + _("new branch name '%s' is invalid"), newname); + break; + + /* not necessary to handle nonfatal cases */ + case VALIDATION_PASS_BRANCH_DOESNT_EXIST: + case VALIDATION_PASS_BRANCH_EXISTS: + case VALIDATION_WARN_BRANCH_EXISTS: + break; + } +} + +/* Validate the old branch name and return the result */ +static enum old_branch_validation_result validate_old_branchname (const char* name, struct strbuf *oldref) { + int bad_ref = strbuf_check_branch_ref(oldref, name); + int branch_exists = ref_exists(oldref->buf); + + if (bad_ref) { + if(branch_exists) + return VALIDATION_1_WARN_BAD_OLD_BRANCH_NAME; + else + return VALIDATION_1_FATAL_INVALID_OL
[PATCH v4 3/3] t/t3200: fix a typo in a test description
Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- t/t3200-branch.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 503a88d02..6c0b7ea4a 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -528,7 +528,7 @@ test_expect_success 'git branch -c -f o/q o/p should work when o/p exists' ' git branch -c -f o/q o/p ' -test_expect_success 'git branch -c qq rr/qq should fail when r exists' ' +test_expect_success 'git branch -c qq rr/qq should fail when rr exists' ' git branch qq && git branch rr && test_must_fail git branch -c qq rr/qq -- 2.16.1.291.g4437f3f13
Re: [PATCH] t/known-leaky: add list of known-leaky test scripts
On Wednesday 21 February 2018 02:14 AM, Martin Ågren wrote: > To sum up: I probably won't be looking into Travis-ing such a blacklist > in the near future. > Just thinking out loud, how about having a white-list instead of a black-list and using it to run only those tests in the white list. Something like, t/white_list t t0001 To run -- cd t/ for test in $(cat white_list) do white_list_tests="$white_list_tests $test*" done make SANITIZE=leak $white_list_tests -- Kaartic QUOTE --- “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - Joel Spolsky signature.asc Description: OpenPGP digital signature
Re: [PATCH v2] Fix misconversion of gitsubmodule.txt
Hi, On Wednesday 21 February 2018 12:20 AM, Stefan Beller wrote: > Kaartic was the last to touch that file, > (as found via git log origin/master -- Documentation/gitsubmodules.txt), > sorry I did not find this in the review. > "Non-ASCII characters" made me dig into to this a little deeper as I generally don't use them particularly for text files. "git blame" points at d48034551 (submodules: overhaul documentation, 2017-06-22) as the offending comment. > Thanks for the patch! Yeah, nice catch! -- Kaartic QUOTE “It is impossible to live without failing at something, unless you live so cautiously that you might as well not have lived at all – in which case, you fail by default.” - J. K. Rowling signature.asc Description: OpenPGP digital signature
Re: Bug/comment
On Tuesday 30 January 2018 05:32 AM, Andrew Ardill wrote: > Hi Ilija, > > On 30 January 2018 at 10:21, Ilija Peceljwrote: >> Though it might not be considered a bug 'per se' it is definitely wired. >> Namely, when you type 'yes' word and hit enter in git bash for widnows, the >> process enters infinite loop and just prints 'y' letter in new line. > > ... > > I agree it's a little weird if you have no idea what it's doing, but > it is very useful and very old, used by many many different scripts > etc, and so unlikely to change. > Just to make things more clear, 'yes' is an UNIX utility (as hinted in the Wikipedia article link) that might come as part of Cygwin and is not a part of Git itself. -- Kaartic QUOTE “It is impossible to live without failing at something, unless you live so cautiously that you might as well not have lived at all – in which case, you fail by default.” - J. K. Rowling WIKIPEDIA: DID YOU KNOW? Only 33% of internet users in India have heard of Wikipedia !! * What do you think could be the reason behind this? * What are ways in which the awareness about Wikipedia in India and other countries be increased ? Reference: * Give your ideas for increasing the awareness of Wikipedia in India and in other countries and get a Grant from the Wikimedia Foundation to bring your idea to life. https://meta.wikimedia.org/wiki/Grants:IdeaLab/Inspire * Know more about the awareness problem of Wikipedia https://meta.wikimedia.org/wiki/New_Readers/Awareness https://meta.wikimedia.org/wiki/New_Readers/Next_steps/Raising_awareness signature.asc Description: OpenPGP digital signature
Re: Bug, git rebase -i does not abort correctly
Hi, On Thursday 25 January 2018 05:43 PM, Duy Nguyen wrote: > rebase scripts are too much for me, so I'll play the user reporting > bugs this time :) > > Instead of doing > > $ git rebase -i --onto origin/master @~3 > > I sometimes accidentally type > > $ git rebase -i origin/master @~3 > > ("rebase -i" is actually an alias, which is why I never forget to type -i) > > Usually the todo list in $EDITOR shows noop, I realize my mistake and > try to abort it by clearing the todo list before saving and closing > $EDITOR. The problem is, HEAD is moved away anyway (to origin/master I > think) For me it left HEAD at @~3. Reading the synopsis of `man git rebase` I could guess that the corresponding abstract form would be, $ git rebase -i > even if rebase is supposed to abort the operation and leave > HEAD untouched. > This might seem to be a bug as the comment in "git-rebase-todo" says, However, if you remove everything, the rebase will be aborted. But "man git rebase" clearly says, If is specified, git rebase will perform an automatic "git checkout " before doing anything else. Otherwise it remains on the current branch. Junio has previously confirmed that "git rebase [-i] " is just a short hand for "git checkout && git rebase [-i] ".[ref 1] So, it's not surprising that it left HEAD at @~3 when you completely removed the contents of git-rebase-todo and exited the editor. Does that help solve your issue? [ref 1]: https://public-inbox.org/git/%3cxmqqpo8387hz@gitster.mtv.corp.google.com%3E -- Kaartic QUOTE “It is impossible to live without failing at something, unless you live so cautiously that you might as well not have lived at all – in which case, you fail by default.” - J. K. Rowling WIKIPEDIA: DID YOU KNOW? Only 33% of internet users in India have heard of Wikipedia !! * What do you think could be the reason behind this? * What are ways in which the awareness about Wikipedia in India and other countries be increased ? Reference: * Give your ideas for increasing the awareness of Wikipedia in India and in other countries and get a Grant from the Wikimedia Foundation to bring your idea to life. https://meta.wikimedia.org/wiki/Grants:IdeaLab/Inspire * Know more about the awareness problem of Wikipedia https://meta.wikimedia.org/wiki/New_Readers/Awareness https://meta.wikimedia.org/wiki/New_Readers/Next_steps/Raising_awareness -- KaarticHi, I seem to able to reproduce your issue. More comments inline. On Thursday 25 January 2018 05:43 PM, Duy Nguyen wrote: > rebase scripts are too much for me, so I'll play the user reporting > bugs this time :) > > Instead of doing > > $ git rebase -i --onto origin/master @~3 > > I sometimes accidentally type > > $ git rebase -i origin/master @~3 > > ("rebase -i" is actually an alias, which is why I never forget to type -i) > > Usually the todo list in $EDITOR shows noop, I realize my mistake and > try to abort it by clearing the todo list before saving and closing > $EDITOR. The problem is, HEAD is moved away anyway (to origin/master I > think) For me it left HEAD at @~3. Reading the synopsis of `man git rebase` I could guess that the corresponding abstract form would be, $ git rebase -i > even if rebase is supposed to abort the operation and leave > HEAD untouched. > This might seem to be a bug as the comment in "git-rebase-todo" says, However, if you remove everything, the rebase will be aborted. But "man git rebase" clearly says, If is specified, git rebase will perform an automatic git checkout before doing anything else. Otherwise it remains on the current branch. Surprisingly when git-rebase-todo's content is only a "noop" just closing the editor without removing the contents also seems to be lying as it checks -- Kaartic QUOTE “It is impossible to live without failing at something, unless you live so cautiously that you might as well not have lived at all – in which case, you fail by default.” - J. K. Rowling WIKIPEDIA: DID YOU KNOW? Only 33% of internet users in India have heard of Wikipedia !! * What do you think could be the reason behind this? * What are ways in which the awareness about Wikipedia in India and other countries be increased ? Reference: * Give your ideas for increasing the awareness of Wikipedia in India and in other countries and get a Grant from the Wikimedia Foundation to bring your idea to life. https://meta.wikimedia.org/wiki/Grants:IdeaLab/Inspire * Know more about the awareness problem of Wikipedia https://meta.wikimedia.org/wiki/New_Readers/Awareness https://meta.wikimedia.org/wiki/New_Readers/Next_steps/Raising_awareness
Re: [PATCH v3 0/2] Doc/submodules: a few updates
On Wednesday 17 January 2018 01:32 AM, Junio C Hamano wrote: > I had a few "Huh?" > moments while reading the resulting text, but nothing show-stopping. > It always happens when there are people around who are trying to be over careful :) Anyways, it's only now that I remember that I've missed a change that I thought of doing. The change is about clarifying what a "de-initialized" submodule means and what an "inactive" submodule means and how they work together (IIUC, a submodule has not active flag when its deinitialized). I foresee people confusing 'init' and 'active'. So, I thought the documentation should be helpful enough in that aspect. Documenation/gitsubmodules doesn't seem to be talking much about 'init'. (It talks about 'active' a lot after these patches :) Now I think it's better to do that as separate change and move this forward as I don't want to make this clumsy anymore. Please let me know, if I'm over thinking things again. :) -- Kaartic signature.asc Description: OpenPGP digital signature
[PATCH v3 2/2] Doc/git-submodule: improve readability and grammar of a sentence
While at it, correctly quote important words. Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- Documentation/git-submodule.txt | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt index ff612001d..71c5618e8 100644 --- a/Documentation/git-submodule.txt +++ b/Documentation/git-submodule.txt @@ -70,8 +70,8 @@ status [--cached] [--recursive] [--] [...]:: Show the status of the submodules. This will print the SHA-1 of the currently checked out commit for each submodule, along with the submodule path and the output of 'git describe' for the - SHA-1. Each SHA-1 will be prefixed with `-` if the submodule is not - initialized, `+` if the currently checked out submodule commit + SHA-1. Each SHA-1 will possibly be prefixed with `-` if the submodule is + not initialized, `+` if the currently checked out submodule commit does not match the SHA-1 found in the index of the containing repository and `U` if the submodule has merge conflicts. + @@ -132,15 +132,15 @@ expects by cloning missing submodules and updating the working tree of the submodules. The "updating" can be done in several ways depending on command line options and the value of `submodule..update` configuration variable. The command line option takes precedence over -the configuration variable. if neither is given, a checkout is performed. -update procedures supported both from the command line as well as setting -`submodule..update`: +the configuration variable. If neither is given, a 'checkout' is performed. +The 'update' procedures supported both from the command line as well as +through the `submodule..update` configuration are: checkout;; the commit recorded in the superproject will be checked out in the submodule on a detached HEAD. + If `--force` is specified, the submodule will be checked out (using -`git checkout --force` if appropriate), even if the commit specified +`git checkout --force`), even if the commit specified in the index of the containing repository already matches the commit checked out in the submodule. @@ -150,8 +150,8 @@ checked out in the submodule. merge;; the commit recorded in the superproject will be merged into the current branch in the submodule. -The following procedures are only available via the `submodule..update` -configuration variable: +The following 'update' procedures are only available via the +`submodule..update` configuration variable: custom command;; arbitrary shell command that takes a single argument (the sha1 of the commit recorded in the -- 2.16.0.rc1.281.g969645f98
[PATCH v3 1/2] Doc/gitsubmodules: make some changes to improve readability and syntax
* Only mention porcelain commands in examples * Split a sentence for better readability * Add missing apostrophes * Clearly specify the advantages of using submodules * Avoid abbreviations * Use "Git" consistently * Improve readability of certain lines * Clarify when a submodule is considered active Helped-by: Eric Sunshine <sunsh...@sunshineco.com> Helped-by: Stefan Beller <sbel...@google.com> Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- Documentation/gitsubmodules.txt | 100 +++- 1 file changed, 79 insertions(+), 21 deletions(-) diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt index 46cf120f6..4d6c17782 100644 --- a/Documentation/gitsubmodules.txt +++ b/Documentation/gitsubmodules.txt @@ -36,8 +36,8 @@ The `gitlink` entry contains the object name of the commit that the superproject expects the submodule’s working directory to be at. The section `submodule.foo.*` in the `.gitmodules` file gives additional -hints to Gits porcelain layer such as where to obtain the submodule via -the `submodule.foo.url` setting. +hints to Git's porcelain layer. For example, the `submodule.foo.url` +setting specifies where to obtain the submodule. Submodules can be used for at least two different use cases: @@ -51,18 +51,21 @@ Submodules can be used for at least two different use cases: 2. Splitting a (logically single) project into multiple repositories and tying them back together. This can be used to - overcome current limitations of Gits implementation to have + overcome current limitations of Git's implementation to have finer grained access: -* Size of the git repository: +* Size of the Git repository: In its current form Git scales up poorly for large repositories containing content that is not compressed by delta computation between trees. - However you can also use submodules to e.g. hold large binary assets - and these repositories are then shallowly cloned such that you do not + For example, you can use submodules to hold large binary assets + and these repositories can be shallowly cloned such that you do not have a large history locally. * Transfer size: In its current form Git requires the whole working tree present. It does not allow partial trees to be transferred in fetch or clone. + If the project you work on consists of multiple repositories tied + together as submodules in a superproject, you can avoid fetching the + working trees of the repositories you are not interested in. * Access control: By restricting user access to submodules, this can be used to implement read/write policies for different users. @@ -73,9 +76,10 @@ The configuration of submodules Submodule operations can be configured using the following mechanisms (from highest to lowest precedence): - * The command line for those commands that support taking submodule specs. - Most commands have a boolean flag '--recurse-submodules' whether to - recurse into submodules. Examples are `ls-files` or `checkout`. + * The command line for those commands that support taking submodules + as part of their pathspecs. Most commands have a boolean flag + `--recurse-submodules` which specify whether to recurse into submodules. + Examples are `grep` and `checkout`. Some commands take enums, such as `fetch` and `push`, where you can specify how submodules are affected. @@ -87,8 +91,8 @@ Submodule operations can be configured using the following mechanisms For example an effect from the submodule's `.gitignore` file would be observed when you run `git status --ignore-submodules=none` in the superproject. This collects information from the submodule's working -directory by running `status` in the submodule, which does pay attention -to its `.gitignore` file. +directory by running `status` in the submodule while paying attention +to the `.gitignore` file of the submodule. + The submodule's `$GIT_DIR/config` file would come into play when running `git push --recurse-submodules=check` in the superproject, as this would @@ -97,20 +101,20 @@ remotes are configured in the submodule as usual in the `$GIT_DIR/config` file. * The configuration file `$GIT_DIR/config` in the superproject. - Typical configuration at this place is controlling if a submodule - is recursed into at all via the `active` flag for example. + Git only recurses into active submodules (see "ACTIVE SUBMODULES" + section below). + If the submodule is not yet initialized, then the configuration -inside the submodule does not exist yet, so configuration where to +inside the submodule does not exist yet, so where to obtain the submodule from is configured here for example. - * the `.gitmodules` file inside the superproject. Additionally to the - required mapping between submod
[PATCH v3 0/2] Doc/submodules: a few updates
Quoting from v1, These are just a few improvements that I thought would make the documentation related to submodules a little better in various way such as readability, consistency etc., These were things I noticed while reading thise documents. Changes since v2: - Made some changes suggested by Stefan. - A few more that caught my eyes. Inter diff between v2 and v3: diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt index 801d291ca..71c5618e8 100644 --- a/Documentation/git-submodule.txt +++ b/Documentation/git-submodule.txt @@ -70,8 +70,8 @@ status [--cached] [--recursive] [--] [...]:: Show the status of the submodules. This will print the SHA-1 of the currently checked out commit for each submodule, along with the submodule path and the output of 'git describe' for the - SHA-1. Each SHA-1 will be prefixed with `-` if the submodule is not - initialized, `+` if the currently checked out submodule commit + SHA-1. Each SHA-1 will possibly be prefixed with `-` if the submodule is + not initialized, `+` if the currently checked out submodule commit does not match the SHA-1 found in the index of the containing repository and `U` if the submodule has merge conflicts. + diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt index ce2369c2d..47bbc62e8 100644 --- a/Documentation/gitsubmodules.txt +++ b/Documentation/gitsubmodules.txt @@ -101,7 +101,7 @@ remotes are configured in the submodule as usual in the `$GIT_DIR/config` file. * The configuration file `$GIT_DIR/config` in the superproject. - Git only recurses into active submodules (see 'ACTIVE SUBMODULES' + Git only recurses into active submodules (see "ACTIVE SUBMODULES" section below). + If the submodule is not yet initialized, then the configuration @@ -164,52 +164,59 @@ from another repository. To completely remove a submodule, manually delete `$GIT_DIR/modules//`. -Active submodules +ACTIVE SUBMODULES - A submodule is considered active, - (a) if `submodule..active` is set + (a) if `submodule..active` is set to `true` or - (b) if the submodules path matches the pathspec in `submodule.active` + (b) if the submodule's path matches the pathspec in `submodule.active` or (c) if `submodule..url` is set. +and these are evaluated in this order. + For example: -[submodule "foo"] -active = false -url = https://example.org/foo -[submodule "bar"] -active = true -url = https://example.org/bar -[submodule "baz"] -url = https://example.org/baz + [submodule "foo"] +active = false +url = https://example.org/foo + [submodule "bar"] +active = true +url = https://example.org/bar + [submodule "baz"] +url = https://example.org/baz -In the above config only the submodule bar and baz are active, -bar due to (a) and baz due to (c). +In the above config only the submodule 'bar' and 'baz' are active, +'bar' due to (a) and 'baz' due to (c). 'foo' is inactive because +(a) takes precedence over (c). -Note that '(c)' is a historical artefact and will be ignored if the -pathspec set in (b) excludes the submodule. For example: +Note that (c) is a historical artefact and will be ignored if the +(a) and (b) specify that the submodule is not active. In other words, +if we have an `submodule..active` set to `false` or if the +submodule's path is excluded in the pathspec in `submodule.active`, the +url doesn't matter whether it is present or not. This is illustrated in +the example that follows. -[submodule "foo"] -active = true -url = https://example.org/foo -[submodule "bar"] -url = https://example.org/bar -[submodule "baz"] -url = https://example.org/baz -[submodule "bob"] -ignore = true -[submodule] -active = b* -active = (:exclude) baz + [submodule "foo"] +active = true +url = https://example.org/foo + [submodule "bar"] +url = https://example.org/bar + [submodule "baz"] +url = https://example.org/baz + [submodule "bob"] +ignore = true + [submodule] +active = b* +active = :(exclude) baz -In here all submodules except baz (foo, bar, bob) are active. +In here all submodules except 'baz' (foo, bar, bob) are active. 'foo' due to its own active flag and all the others due to the submodule active pathspec, which specifies that any submodule -starting with 'b' except 'baz' are also active, no matter if -the .url field is present. +starting with 'b' except 'baz' are also active, regardless of the +presence of the .url field. Workflow for a third party library -- Kaartic Sivaraam (2): Doc/gitsubmodules: make som
[PATCH v2 0/2] Doc/submodules: a few updates
+} directory may be there as after deinitializing the [-git-]{+Git+} directory is kept around. The directory which is supposed to be the working directory is empty instead. + A submodule can be deinitialized by running `git submodule deinit`. @@ -164,6 +164,53 @@ from another repository. To completely remove a submodule, manually delete `$GIT_DIR/modules//`. {+Active submodules+} {+-+} {+A submodule is considered active,+} {+ (a) if `submodule..active` is set+} {+ or+} {+ (b) if the submodules path matches the pathspec in `submodule.active`+} {+ or+} {+ (c) if `submodule..url` is set.+} {+For example:+} {+[submodule "foo"]+} {+active = false+} {+url = https://example.org/foo+} {+[submodule "bar"]+} {+active = true+} {+url = https://example.org/bar+} {+[submodule "baz"]+} {+url = https://example.org/baz+} {+In the above config only the submodule bar and baz are active,+} {+bar due to (a) and baz due to (c).+} {+Note that '(c)' is a historical artefact and will be ignored if the+} {+pathspec set in (b) excludes the submodule. For example:+} {+[submodule "foo"]+} {+active = true+} {+url = https://example.org/foo+} {+[submodule "bar"]+} {+url = https://example.org/bar+} {+[submodule "baz"]+} {+url = https://example.org/baz+} {+[submodule "bob"]+} {+ignore = true+} {+[submodule]+} {+active = b*+} {+active = (:exclude) baz+} {+In here all submodules except baz (foo, bar, bob) are active.+} {+'foo' due to its own active flag and all the others due to the+} {+submodule active pathspec, which specifies that any submodule+} {+starting with 'b' except 'baz' are also active, no matter if+} {+the .url field is present.+} Workflow for a third party library -- Kaartic Sivaraam (2): Doc/gitsubmodules: make some changes to improve readability and syntax Doc/git-submodule: improve readability and grammar of a sentence Documentation/git-submodule.txt | 12 +++--- Documentation/gitsubmodules.txt | 93 +++-- 2 files changed, 78 insertions(+), 27 deletions(-) -- 2.16.0.rc0.223.g4a4ac8367
[PATCH v2 2/2] Doc/git-submodule: improve readability and grammar of a sentence
While at it, correctly quote important words. Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- Documentation/git-submodule.txt | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt index ff612001d..801d291ca 100644 --- a/Documentation/git-submodule.txt +++ b/Documentation/git-submodule.txt @@ -132,15 +132,15 @@ expects by cloning missing submodules and updating the working tree of the submodules. The "updating" can be done in several ways depending on command line options and the value of `submodule..update` configuration variable. The command line option takes precedence over -the configuration variable. if neither is given, a checkout is performed. -update procedures supported both from the command line as well as setting -`submodule..update`: +the configuration variable. If neither is given, a 'checkout' is performed. +The 'update' procedures supported both from the command line as well as +through the `submodule..update` configuration are: checkout;; the commit recorded in the superproject will be checked out in the submodule on a detached HEAD. + If `--force` is specified, the submodule will be checked out (using -`git checkout --force` if appropriate), even if the commit specified +`git checkout --force`), even if the commit specified in the index of the containing repository already matches the commit checked out in the submodule. @@ -150,8 +150,8 @@ checked out in the submodule. merge;; the commit recorded in the superproject will be merged into the current branch in the submodule. -The following procedures are only available via the `submodule..update` -configuration variable: +The following 'update' procedures are only available via the +`submodule..update` configuration variable: custom command;; arbitrary shell command that takes a single argument (the sha1 of the commit recorded in the -- 2.16.0.rc0.223.g4a4ac8367
[PATCH v2 1/2] Doc/gitsubmodules: make some changes to improve readability and syntax
* Only mention porcelain commands in examples * Split a sentence for better readability * Add missing apostrophes * Clearly specify the advantages of using submodules * Avoid abbreviations * Use "Git" consistently * Improve readability of certain lines * Clarify when a submodule is considered active Helped-by: Eric Sunshine <sunsh...@sunshineco.com> Helped-by: Stefan Beller <sbel...@google.com> Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- Documentation/gitsubmodules.txt | 93 +++-- 1 file changed, 72 insertions(+), 21 deletions(-) diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt index 46cf120f6..ce2369c2d 100644 --- a/Documentation/gitsubmodules.txt +++ b/Documentation/gitsubmodules.txt @@ -36,8 +36,8 @@ The `gitlink` entry contains the object name of the commit that the superproject expects the submodule’s working directory to be at. The section `submodule.foo.*` in the `.gitmodules` file gives additional -hints to Gits porcelain layer such as where to obtain the submodule via -the `submodule.foo.url` setting. +hints to Git's porcelain layer. For example, the `submodule.foo.url` +setting specifies where to obtain the submodule. Submodules can be used for at least two different use cases: @@ -51,18 +51,21 @@ Submodules can be used for at least two different use cases: 2. Splitting a (logically single) project into multiple repositories and tying them back together. This can be used to - overcome current limitations of Gits implementation to have + overcome current limitations of Git's implementation to have finer grained access: -* Size of the git repository: +* Size of the Git repository: In its current form Git scales up poorly for large repositories containing content that is not compressed by delta computation between trees. - However you can also use submodules to e.g. hold large binary assets - and these repositories are then shallowly cloned such that you do not + For example, you can use submodules to hold large binary assets + and these repositories can be shallowly cloned such that you do not have a large history locally. * Transfer size: In its current form Git requires the whole working tree present. It does not allow partial trees to be transferred in fetch or clone. + If the project you work on consists of multiple repositories tied + together as submodules in a superproject, you can avoid fetching the + working trees of the repositories you are not interested in. * Access control: By restricting user access to submodules, this can be used to implement read/write policies for different users. @@ -73,9 +76,10 @@ The configuration of submodules Submodule operations can be configured using the following mechanisms (from highest to lowest precedence): - * The command line for those commands that support taking submodule specs. - Most commands have a boolean flag '--recurse-submodules' whether to - recurse into submodules. Examples are `ls-files` or `checkout`. + * The command line for those commands that support taking submodules + as part of their pathspecs. Most commands have a boolean flag + `--recurse-submodules` which specify whether to recurse into submodules. + Examples are `grep` and `checkout`. Some commands take enums, such as `fetch` and `push`, where you can specify how submodules are affected. @@ -87,8 +91,8 @@ Submodule operations can be configured using the following mechanisms For example an effect from the submodule's `.gitignore` file would be observed when you run `git status --ignore-submodules=none` in the superproject. This collects information from the submodule's working -directory by running `status` in the submodule, which does pay attention -to its `.gitignore` file. +directory by running `status` in the submodule while paying attention +to the `.gitignore` file of the submodule. + The submodule's `$GIT_DIR/config` file would come into play when running `git push --recurse-submodules=check` in the superproject, as this would @@ -97,20 +101,20 @@ remotes are configured in the submodule as usual in the `$GIT_DIR/config` file. * The configuration file `$GIT_DIR/config` in the superproject. - Typical configuration at this place is controlling if a submodule - is recursed into at all via the `active` flag for example. + Git only recurses into active submodules (see 'ACTIVE SUBMODULES' + section below). + If the submodule is not yet initialized, then the configuration -inside the submodule does not exist yet, so configuration where to +inside the submodule does not exist yet, so where to obtain the submodule from is configured here for example. - * the `.gitmodules` file inside the superproject. Additionally to the - required mapping between submodule's name
Re: [PATCH 6/8] Doc/gitsubmodules: improve readability of certain lines
On Wednesday 10 January 2018 01:01 AM, Stefan Beller wrote: The submodule's `$GIT_DIR/config` file would come into play when running `git push --recurse-submodules=check` in the superproject, as this would @@ -107,13 +108,13 @@ If the submodule is not yet initialized, then the configuration inside the submodule does not exist yet, so configuration where to obtain the submodule from is configured here for example. >> >> I caught this in the context while replying. "so configuration where to >> obtain the submodule from is configured here for example." doesn't seem >> to read well. Maybe removing configuration from the sentence will make >> it sound better? >> I'm going to make this change. >> - * the `.gitmodules` file inside the superproject. Additionally to the - required mapping between submodule's name and path, a project usually + * The `.gitmodules` file inside the superproject. Additionally, if mapping + is required between a submodule's name and its path, a project usually >>> >>> This changes meaning, originally it tries to say: >>> >>> * it requires mapping path <-> names. >> >> I get this ... >> >>> * but there can be more. >> >> ... but not this. Did the previous version really try to say this? >> Anyways how does this sound? > > Well that was me being very sloppy trying to say that there might be > submodule..{url, ignored, shallow} settings which just happen to > be there. > >> * The `.gitmodules` file inside the superproject. A project usually >> uses this file to suggest defaults for the upstream collection >> of repositories for the mapping that is required between a >> submodule's name and its path. >> >> I think it conveys the "it requires mapping path <-> names." correctly >> but doesn't convey the "but there can be more." part. I'm not sure how >> to get that into the sentence, correctly. > > I did not consider that part the important part, hence my sloppiness. > Sorry for the confusion. > > My main point was to say that the mapping is the important part and > must be found in the .gitmodules file, otherwise we do not consider > it a submodule (for whatever "it" is, maybe a gitlink at a path=name). > So, I'm going to use the version that I specified above as I think it seems to convey that clearly (at least to me), The `.gitmodules` file inside the superproject. A project usually uses this file to suggest defaults for the upstream collection of repositories for the mapping that is required between a submodule's name and its path. Let me know, if there are issues. Thanks, Kaartic signature.asc Description: OpenPGP digital signature
Re: [PATCH 4/8] Doc/gitsubmodules: avoid abbreviations
On Wednesday 10 January 2018 12:56 AM, Stefan Beller wrote: > On Tue, Jan 9, 2018 at 8:06 AM, Kaartic Sivaraam > <kaartic.sivar...@gmail.com> wrote: >> On Tuesday 09 January 2018 12:15 AM, Stefan Beller wrote: >>>> >>>> - * The command line for those commands that support taking submodule >>>> specs. >>> >>> ++ The command line for those commands that support taking submodules >>> as part of their pathspecs[1]. >>> ++ >>> ++[1] pathspec is an official term according to `man gitglossary`. >>> >>> Maybe? >>> >> >> Yeah, I actually did think 'specification' wasn't a the best fit for >> this (should have mentioned that somewhere) Now, the real term came out :) >> >> Just to be sure, that "[1] pathspec ..." part goes to the end of the >> document doesn't it? > > The [1] part was just to highlight it for the sake of this discussion; > I would not include it into the man page. > That '++' before it made me think otherwise :) signature.asc Description: OpenPGP digital signature
Re: [PATCH 3/8] Doc/gitsubmodules: specify how submodules help in reduced size
On Wednesday 10 January 2018 12:31 AM, Stefan Beller wrote: > On Tue, Jan 9, 2018 at 8:01 AM, Kaartic Sivaraam > <kaartic.sivar...@gmail.com> wrote: >> On Tuesday 09 January 2018 12:08 AM, Stefan Beller wrote: >>>> diff --git a/Documentation/gitsubmodules.txt >>>> b/Documentation/gitsubmodules.txt >>>> index cb795c6b6..3f73983d5 100644 >>>> --- a/Documentation/gitsubmodules.txt >>>> +++ b/Documentation/gitsubmodules.txt >>>> @@ -63,6 +63,9 @@ Submodules can be used for at least two different use >>>> cases: >>>> * Transfer size: >>>>In its current form Git requires the whole working tree present. It >>>>does not allow partial trees to be transferred in fetch or clone. >>>> + If you have your project as multiple repositories tied together as >>>> + submodules in a superproject, you can avoid fetching the working >>>> + trees of the repositories you are not interested in. >>> >>> You do not fetch a working tree, but a whole repository? >>> >> >> Maybe I misunderstood submodules when I wrote that example. Could you >> help out with a better and precise replacement? > > If your project consists of multiple repositories tied together, some > submodules > may not be of interest for all users, who do not need to fetch such submodule > repositories. > OK, now I get why I couldn't get your point. I actually was thinking of the version of the message I had tweaked for v2 when reading your message. It doesn't have the confusing meaning. It actually reads, ... If the project you work on consists of multiple repositories tied together as submodules in a superproject, you can avoid fetching the working trees of the repositories you are not interested in. So, my version takes the perspective of the person who gains the advantage of having to clone unnecessary repos. And yours, the perspective of the person who gives the consumer of the repo that advantage. Both sound nice to me. But if mine doesn't sound nice to you, let me know so that I could replace it with your version. >> Just putting in some context as to why I did this change, I thought this >> was the only thing that lacked an example and wanted to make it consistent. > > Oh, sure I like the example; I was just worried about the wording, as a > worktree > is part of a repository, and the repository is the whole thing. In the > current situation > you can only fetch all-or-nothing, specifically you cannot fetch "just > the worktree" > (a shallow clone/fetch is the closest to that, but that still has the > same amount of > information the .git dir, than in the working tree) > I get that! signature.asc Description: OpenPGP digital signature
Re: [PATCH 0/8] Doc/submodules: a few updates
On Tuesday 09 January 2018 12:38 AM, Stefan Beller wrote: > On Sat, Jan 6, 2018 at 10:46 AM, Kaartic Sivaraam > <kaartic.sivar...@gmail.com> wrote: > > While small patches are really appreciated for code (bisect, automated > testing, and > the general difficulty to reason about code, as a very small change > may affect the whole > code base), I am not sure if they benefit in documentation. > Documentation is a rather > local human readable thing, so by changing one sentence we don't > affect the understanding > of documentation at a completely unrelated place. > > Also it helps to read more than just sentence fragments, i.e. I tried > looking at the > whole paragraph for review. May I suggest to squash them all and > resend as one patch? > I wouldn't mind that. I thought it might be easy to find to find the parts I changed when the patches are small. So, I sent them without squashing them together. In case you feel it's not worth, let me know so I'll squash them in. BTW, in case I did squash them in, would it be nice to keep the commit subjects of the current patch series as bullet points in the unified commit message? > >> >> I based these patches on top of 'master'. > > I am not aware of other submodule patches affecting documentation in > master..pu, > so this should be easy to merge. > >> >> Apart from the changes, I saw a few things that needed >> improvement/clarification >> but wasn't able to do that myself due to my limited knowledge of submodules. >> They >> are listed below. I'll add in patches for them if they are correctly >> clarified. >> >> >> 1. >> >> man gitsubmodules >> >>· The configuration file $GIT_DIR/config in the superproject. >> Typical configuration at this place is controlling if a submodule is >>recursed into at all via the active flag for example. >> >>If the submodule is not yet initialized, then the configuration >> inside the submodule does not exist yet, so configuration where to >>obtain the submodule from is configured here for example. >> >> What's the "active flag" mentioned above? Also I find the phrase "is >> recursed into at all" >> to be a little slippery. How could it be improved? > > There are multiple ways to indicate if a submodule is "active", i.e. if Git is > supposed to pay attentio. Historically we had to set the > submodule..url flag in the config, but last year Brandon added > submodule.active as well as submodule..active which supersede > the .url flag. > > (See is_submodule_active() in submodule.c to see the definitive answer to > "should Git pay attention?") > https://github.com/git/git/blob/master/submodule.c#L224 > Thanks for the info! > I wonder if this indicates a lack of documentation when the active > flags were introduced. > They are found in 'man git config', but maybe we need to spell them > out explicitly > in the submodule related docs. > Possibly. So, why not in Documentation/gitsubmodules! Here's a replaced version of that paragraph, * The configuration file `$GIT_DIR/config` in the superproject. Typically this file is used to specify whether the submodule is recursed into at all via the `active` flag for example. A submodule is considered active if `submodule..url` is set or if the submodules path is present in `submodule.active` or if `submodule..url` is set. >> 2. >> >> man git submodule >> >>update >>... >> >>checkout >> >> >>If --force is specified, the submodule will be checked out >> (using git checkout --force if appropriate), even if the commit >>specified in the index of the containing repository already >> matches the commit checked out in the submodule. >> >> I'm not sure this is conveying all the information it should be conveying. >> It seems to making the user wonder, "How at all does 'git submodule update >> --force' >> differs from 'git submodule update'?" also "using git checkout --force if >> appropriate" >> seems to be invoking all sorts confusion as "appropriate" is superfluous. > > When "submodule update" is invoked with the `--force` flag, that flag is > passed > on to the 'checkout' operation. If you do not give the --force, then > the checkout > will also be done without --force. > If that's the case then shouldn't the "if appropriate" part of "(using git checkout --force if appropriate)" be dropped? That seems to make it clear, at least for me. Or is intended as '--force' will not be passed to git checkout all the time? >> >> How could these confusions be clarified? > > I tried giving an alternative snippet above, not sure how else to tell. > -- Kaartic Quote: "Be creative. Be adventurous. Be original. And above all else, be young." - Wonder Woman signature.asc Description: OpenPGP digital signature
Re: [PATCH 6/8] Doc/gitsubmodules: improve readability of certain lines
On Tuesday 09 January 2018 12:19 AM, Stefan Beller wrote: > On Sat, Jan 6, 2018 at 10:46 AM, Kaartic Sivaraam > <kaartic.sivar...@gmail.com> wrote: >> >> - * The command line for those commands that support taking submodule >> - specifications. Most commands have a boolean flag '--recurse-submodules >> - whether to recurse into submodules. Examples are `ls-files` or >> `checkout`. >> + * The command line arguments of those commands that support taking >> submodule >> + specifications. Most commands have a boolean flag '--recurse-submodules' >> + which specify whether they should recurse into submodules. Examples are >> + `ls-files` or `checkout`. >> Some commands take enums, such as `fetch` and `push`, where you can >> specify how submodules are affected. >> >> @@ -90,8 +91,8 @@ Submodule operations can be configured using the following >> mechanisms >> For example an effect from the submodule's `.gitignore` file >> would be observed when you run `git status --ignore-submodules=none` in >> the superproject. This collects information from the submodule's working >> -directory by running `status` in the submodule, which does pay attention >> -to its `.gitignore` file. >> +directory by running `status` in the submodule while paying attention >> +to the `.gitignore` file of the submodule. > > Both are grammatically correct and expressive, thanks! > You're welcome! >> + > > Extra spurious line? > No. That's a "real" plus in the document that's usually present between paragraphs :) I think I now get why Junio suggests people to review patches in context (possibly, by applying them) ;) >> The submodule's `$GIT_DIR/config` file would come into play when running >> `git push --recurse-submodules=check` in the superproject, as this would >> @@ -107,13 +108,13 @@ If the submodule is not yet initialized, then the >> configuration >> inside the submodule does not exist yet, so configuration where to >> obtain the submodule from is configured here for example. >> I caught this in the context while replying. "so configuration where to obtain the submodule from is configured here for example." doesn't seem to read well. Maybe removing configuration from the sentence will make it sound better? >> - * the `.gitmodules` file inside the superproject. Additionally to the >> - required mapping between submodule's name and path, a project usually >> + * The `.gitmodules` file inside the superproject. Additionally, if mapping >> + is required between a submodule's name and its path, a project usually > > This changes meaning, originally it tries to say: > > * it requires mapping path <-> names. I get this ... > * but there can be more. > ... but not this. Did the previous version really try to say this? Anyways how does this sound? * The `.gitmodules` file inside the superproject. A project usually uses this file to suggest defaults for the upstream collection of repositories for the mapping that is required between a submodule's name and its path. I think it conveys the "it requires mapping path <-> names." correctly but doesn't convey the "but there can be more." part. I'm not sure how to get that into the sentence, correctly. signature.asc Description: OpenPGP digital signature
Re: [PATCH 4/8] Doc/gitsubmodules: avoid abbreviations
On Tuesday 09 January 2018 12:15 AM, Stefan Beller wrote: >> >> - * The command line for those commands that support taking submodule specs. > > ++ The command line for those commands that support taking submodules > as part of their pathspecs[1]. > ++ > ++[1] pathspec is an official term according to `man gitglossary`. > > Maybe? > Yeah, I actually did think 'specification' wasn't a the best fit for this (should have mentioned that somewhere) Now, the real term came out :) Just to be sure, that "[1] pathspec ..." part goes to the end of the document doesn't it? >> - Most commands have a boolean flag '--recurse-submodules' whether to >> - recurse into submodules. Examples are `ls-files` or `checkout`. >> + * The command line for those commands that support taking submodule >> + specifications. Most commands have a boolean flag '--recurse-submodules >> + whether to recurse into submodules. Examples are `ls-files` or >> `checkout`. >> Some commands take enums, such as `fetch` and `push`, where you can >> specify how submodules are affected. >> >> -- >> 2.16.0.rc0.223.g4a4ac8367 >> signature.asc Description: OpenPGP digital signature
Re: [PATCH 3/8] Doc/gitsubmodules: specify how submodules help in reduced size
On Tuesday 09 January 2018 12:08 AM, Stefan Beller wrote: >> diff --git a/Documentation/gitsubmodules.txt >> b/Documentation/gitsubmodules.txt >> index cb795c6b6..3f73983d5 100644 >> --- a/Documentation/gitsubmodules.txt >> +++ b/Documentation/gitsubmodules.txt >> @@ -63,6 +63,9 @@ Submodules can be used for at least two different use >> cases: >> * Transfer size: >>In its current form Git requires the whole working tree present. It >>does not allow partial trees to be transferred in fetch or clone. >> + If you have your project as multiple repositories tied together as >> + submodules in a superproject, you can avoid fetching the working >> + trees of the repositories you are not interested in. > > You do not fetch a working tree, but a whole repository? > Maybe I misunderstood submodules when I wrote that example. Could you help out with a better and precise replacement? Just putting in some context as to why I did this change, I thought this was the only thing that lacked an example and wanted to make it consistent. -- Kaartic signature.asc Description: OpenPGP digital signature
Re: [PATCH 2/8] Doc/gitsubmodules: clearly specify advantage of submodule
>>content that is not compressed by delta computation between trees. >> - However you can also use submodules to e.g. hold large binary assets >> + Therefore you can use submodules to hold large binary assets > > If this improves readability by a lot, I'd be all for it. But this use > case is just > exemplary. There are also cases of submodules that do not contain big files, > but e.g. have a lengthy history with lots of small files. > So I don't know, as I would want to keep emphasized that this is just > an example. > You're right I actually screwed up by emphasizing that was the only use case it was envisioned for. Fixed it by replacing "Therefore" with " For example, ..." Thanks, Kaartic signature.asc Description: OpenPGP digital signature
[BUG] "git stash -p" doesn't work well when splitting nearby changes
I recently made two changes to nearby lines (approx. 3 lines b/w them) in a file and wanted to stash one part of it and not the other. I tried to use "git stash -p" for this. I split the hunk of concern (of course, they came out as a single hunk) and gave 'y' for one and 'n' for the other. The stash was created successfully but the discarding that change from the work tree failed with the following error, error: patch failed: Documentation/gitsubmodules.txt:57 error: Documentation/gitsubmodules.txt: patch does not apply Cannot remove worktree changes I guess this a bug that has been for a very long time now (I generally use the one built from 'next' but this error occurs even if I use 2.11)? Or is this intentional? I couldn't glean any reason for that, though. In case you need a reproduction recipe, git init stash-patch-test && cd stash-patch-test && echo 'Let us put some lines to ensure Git would allow us to split a change we make to this text. It should be a little longer, I guess. A little more. Almost there. And DONE.' >test-files && git add . && git commit -m "Initial commit" Apply this patch or do something similar to what it does, diff --git a/test-files b/test-files index daa67cf..d3142e7 100644 --- a/test-files +++ b/test-files @@ -1,7 +1,7 @@ -Let us put some lines to ensure +Let's put some lines to ensure Git would allow us to split a change we make -to this text. +to this text It should be a little longer, I guess. A little more. Almost there. Then, $ git stash -p diff --git a/test-files b/test-files index daa67cf..d3142e7 100644 --- a/test-files +++ b/test-files @@ -1,7 +1,7 @@ -Let us put some lines to ensure +Let's put some lines to ensure Git would allow us to split a change we make -to this text. +to this text It should be a little longer, I guess. A little more. Almost there. Stash this hunk [y,n,q,a,d,/,s,e,?]? s Split into 2 hunks. @@ -1,3 +1,3 @@ -Let us put some lines to ensure +Let's put some lines to ensure Git would allow us to split a change we make Stash this hunk [y,n,q,a,d,/,j,J,g,e,?]? n @@ -2,6 +2,6 @@ Git would allow us to split a change we make -to this text. +to this text It should be a little longer, I guess. A little more. Almost there. Stash this hunk [y,n,q,a,d,/,K,g,e,?]? y Saved working directory and index state WIP on master: f0dff20 Initial commit error: patch failed: test-files:1 error: test-files: patch does not apply Cannot remove worktree changes -- Kaartic Quote: "Be creative. Be adventurous. Be original. And above all else, be young." - Wonder Woman signature.asc Description: OpenPGP digital signature
[PATCH 8/8] Doc/git-submodule: correctly quote important words
Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- Documentation/git-submodule.txt | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt index befbccde6..5c4d941cc 100644 --- a/Documentation/git-submodule.txt +++ b/Documentation/git-submodule.txt @@ -132,8 +132,8 @@ expects by cloning missing submodules and updating the working tree of the submodules. The "updating" can be done in several ways depending on command line options and the value of `submodule..update` configuration variable. The command line option takes precedence over -the configuration variable. If neither is given, a checkout is performed. -The update procedures supported both from the command line as well as +the configuration variable. If neither is given, a 'checkout' is performed. +The 'update' procedures supported both from the command line as well as through the `submodule..update` configuration are: checkout;; the commit recorded in the superproject will be @@ -150,8 +150,8 @@ checked out in the submodule. merge;; the commit recorded in the superproject will be merged into the current branch in the submodule. -The following procedures are only available via the `submodule..update` -configuration variable: +The following 'update' procedures are only available via the +`submodule..update` configuration variable: custom command;; arbitrary shell command that takes a single argument (the sha1 of the commit recorded in the -- 2.16.0.rc0.223.g4a4ac8367
[PATCH 6/8] Doc/gitsubmodules: improve readability of certain lines
Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- Documentation/gitsubmodules.txt | 19 ++- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt index 745a3838e..339fb73db 100644 --- a/Documentation/gitsubmodules.txt +++ b/Documentation/gitsubmodules.txt @@ -76,9 +76,10 @@ The configuration of submodules Submodule operations can be configured using the following mechanisms (from highest to lowest precedence): - * The command line for those commands that support taking submodule - specifications. Most commands have a boolean flag '--recurse-submodules - whether to recurse into submodules. Examples are `ls-files` or `checkout`. + * The command line arguments of those commands that support taking submodule + specifications. Most commands have a boolean flag '--recurse-submodules' + which specify whether they should recurse into submodules. Examples are + `ls-files` or `checkout`. Some commands take enums, such as `fetch` and `push`, where you can specify how submodules are affected. @@ -90,8 +91,8 @@ Submodule operations can be configured using the following mechanisms For example an effect from the submodule's `.gitignore` file would be observed when you run `git status --ignore-submodules=none` in the superproject. This collects information from the submodule's working -directory by running `status` in the submodule, which does pay attention -to its `.gitignore` file. +directory by running `status` in the submodule while paying attention +to the `.gitignore` file of the submodule. + The submodule's `$GIT_DIR/config` file would come into play when running `git push --recurse-submodules=check` in the superproject, as this would @@ -107,13 +108,13 @@ If the submodule is not yet initialized, then the configuration inside the submodule does not exist yet, so configuration where to obtain the submodule from is configured here for example. - * the `.gitmodules` file inside the superproject. Additionally to the - required mapping between submodule's name and path, a project usually + * The `.gitmodules` file inside the superproject. Additionally, if mapping + is required between a submodule's name and its path, a project usually uses this file to suggest defaults for the upstream collection of repositories. + -This file mainly serves as the mapping between name and path in -the superproject, such that the submodule's Git directory can be +This file mainly serves as the mapping between the name and path of submodules +in the superproject, such that the submodule's Git directory can be located. + If the submodule has never been initialized, this is the only place -- 2.16.0.rc0.223.g4a4ac8367
[PATCH 7/8] Doc/git-submodule: improve readability and grammar of a sentence
Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- Documentation/git-submodule.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt index ff612001d..befbccde6 100644 --- a/Documentation/git-submodule.txt +++ b/Documentation/git-submodule.txt @@ -132,9 +132,9 @@ expects by cloning missing submodules and updating the working tree of the submodules. The "updating" can be done in several ways depending on command line options and the value of `submodule..update` configuration variable. The command line option takes precedence over -the configuration variable. if neither is given, a checkout is performed. -update procedures supported both from the command line as well as setting -`submodule..update`: +the configuration variable. If neither is given, a checkout is performed. +The update procedures supported both from the command line as well as +through the `submodule..update` configuration are: checkout;; the commit recorded in the superproject will be checked out in the submodule on a detached HEAD. -- 2.16.0.rc0.223.g4a4ac8367
[PATCH 4/8] Doc/gitsubmodules: avoid abbreviations
Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- Documentation/gitsubmodules.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt index 3f73983d5..e3c798d2a 100644 --- a/Documentation/gitsubmodules.txt +++ b/Documentation/gitsubmodules.txt @@ -76,9 +76,9 @@ The configuration of submodules Submodule operations can be configured using the following mechanisms (from highest to lowest precedence): - * The command line for those commands that support taking submodule specs. - Most commands have a boolean flag '--recurse-submodules' whether to - recurse into submodules. Examples are `ls-files` or `checkout`. + * The command line for those commands that support taking submodule + specifications. Most commands have a boolean flag '--recurse-submodules + whether to recurse into submodules. Examples are `ls-files` or `checkout`. Some commands take enums, such as `fetch` and `push`, where you can specify how submodules are affected. -- 2.16.0.rc0.223.g4a4ac8367
[PATCH 5/8] Doc/gitsubmodules: use "Git directory" consistently
Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- Documentation/gitsubmodules.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt index e3c798d2a..745a3838e 100644 --- a/Documentation/gitsubmodules.txt +++ b/Documentation/gitsubmodules.txt @@ -113,7 +113,7 @@ obtain the submodule from is configured here for example. of repositories. + This file mainly serves as the mapping between name and path in -the superproject, such that the submodule's git directory can be +the superproject, such that the submodule's Git directory can be located. + If the submodule has never been initialized, this is the only place -- 2.16.0.rc0.223.g4a4ac8367
[PATCH 3/8] Doc/gitsubmodules: specify how submodules help in reduced size
Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- Documentation/gitsubmodules.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt index cb795c6b6..3f73983d5 100644 --- a/Documentation/gitsubmodules.txt +++ b/Documentation/gitsubmodules.txt @@ -63,6 +63,9 @@ Submodules can be used for at least two different use cases: * Transfer size: In its current form Git requires the whole working tree present. It does not allow partial trees to be transferred in fetch or clone. + If you have your project as multiple repositories tied together as + submodules in a superproject, you can avoid fetching the working + trees of the repositories you are not interested in. * Access control: By restricting user access to submodules, this can be used to implement read/write policies for different users. -- 2.16.0.rc0.223.g4a4ac8367
[PATCH 2/8] Doc/gitsubmodules: clearly specify advantage of submodule
Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- Documentation/gitsubmodules.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt index bf46b0fb5..cb795c6b6 100644 --- a/Documentation/gitsubmodules.txt +++ b/Documentation/gitsubmodules.txt @@ -57,7 +57,7 @@ Submodules can be used for at least two different use cases: * Size of the git repository: In its current form Git scales up poorly for large repositories containing content that is not compressed by delta computation between trees. - However you can also use submodules to e.g. hold large binary assets + Therefore you can use submodules to hold large binary assets and these repositories are then shallowly cloned such that you do not have a large history locally. * Transfer size: -- 2.16.0.rc0.223.g4a4ac8367
[PATCH 1/8] Doc/gitsubmodules: split a sentence for better readability
Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- Documentation/gitsubmodules.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt index 46cf120f6..bf46b0fb5 100644 --- a/Documentation/gitsubmodules.txt +++ b/Documentation/gitsubmodules.txt @@ -36,8 +36,8 @@ The `gitlink` entry contains the object name of the commit that the superproject expects the submodule’s working directory to be at. The section `submodule.foo.*` in the `.gitmodules` file gives additional -hints to Gits porcelain layer such as where to obtain the submodule via -the `submodule.foo.url` setting. +hints to Gits porcelain layer. For example, the `submodule.foo.url` +setting specifies where to obtain the submodule. Submodules can be used for at least two different use cases: -- 2.16.0.rc0.223.g4a4ac8367
[PATCH 0/8] Doc/submodules: a few updates
These are just a few improvements that I thought would make the documentation related to submodules a little better in various way such as readability, consistency etc., These were things I noticed while reading thise documents. Sorry, for the highly granular patches. I did the commits as and when I was reading them and tried to keep them focused to one particular change by rebasing them as needed. In case they need some change, let me know. I based these patches on top of 'master'. Apart from the changes, I saw a few things that needed improvement/clarification but wasn't able to do that myself due to my limited knowledge of submodules. They are listed below. I'll add in patches for them if they are correctly clarified. 1. man gitsubmodules · The configuration file $GIT_DIR/config in the superproject. Typical configuration at this place is controlling if a submodule is recursed into at all via the active flag for example. If the submodule is not yet initialized, then the configuration inside the submodule does not exist yet, so configuration where to obtain the submodule from is configured here for example. What's the "active flag" mentioned above? Also I find the phrase "is recursed into at all" to be a little slippery. How could it be improved? 2. man git submodule update ... checkout If --force is specified, the submodule will be checked out (using git checkout --force if appropriate), even if the commit specified in the index of the containing repository already matches the commit checked out in the submodule. I'm not sure this is conveying all the information it should be conveying. It seems to making the user wonder, "How at all does 'git submodule update --force' differs from 'git submodule update'?" also "using git checkout --force if appropriate" seems to be invoking all sorts confusion as "appropriate" is superfluous. How could these confusions be clarified? --- Kaartic Kaartic Sivaraam (8): Doc/gitsubmodules: split a sentence for better readability Doc/gitsubmodules: clearly specify advantage of submodule Doc/gitsubmodules: specify how submodules help in reduced size Doc/gitsubmodules: avoid abbreviations Doc/gitsubmodules: use "Git directory" consistently Doc/gitsubmodules: improve readability of certain lines Doc/git-submodule: improve readability and grammar of a sentence Doc/git-submodule: correctly quote important words Documentation/git-submodule.txt | 10 +- Documentation/gitsubmodules.txt | 28 2 files changed, 21 insertions(+), 17 deletions(-) -- 2.16.0.rc0.223.g4a4ac8367
Re: What's cooking in git.git (Dec 2017, #05; Wed, 27)
On Wednesday 03 January 2018 02:56 PM, Daniel Knittl-Frank wrote: On Wed, Dec 27, 2017 at 10:34 PM, Junio C Hamanowrote: * dk/describe-all-output-fix (2017-12-27) 1 commit - describe: prepend "tags/" when describing tags with embedded name An old regression in "git describe --all $annotated_tag^0" has been fixed. Will merge to 'next'. Shouldn't this be merged to 'maint' since it is a bugfix (for a long standing bug)? Or am I misinterpreting the meaning of the 'maint' branch? IIUC, the pipe line here is something like, [PATCH] | (after the patch gets some consensus when it's not trivial) | V Merge to 'pu' | (after waiting for some time to see if someone shouts about a build failing or complaining a regression about the PATCH in 'pu') | V Merge to 'next' | (after waiting for some time to see if someone shouts about a build failing or complaining a regression about the PATCH in 'next') | / \ / \ / \ / \ | | (if it's a bugfix for (if it's a new feature or an already releasedan enhancement) version of Git) | | V V Merge to 'maint'Merge to 'master' Of course 'maint' and 'master' are not diverged completely. They are 'synced' at times. Disclaimer: I won't say I'm 100% correct with the pipeline. This is just what I've understood in observing the mailing list, the "What's cooking" emails and the history of 'git' in the short time that I've been here. So, there are possibilities that I've said something incorrectly. I guess the "Documentation/howto/maintain-git.txt" document covers it more comprehensively especially "The Policy" section describes the branches more clearly. -- Kaartic Quote: "Be creative. Be adventurous. Be original. And above all else, be young." - Wonder Woman
Re: [ANNOUNCE] Git v2.16.0-rc0
On Friday 29 December 2017 10:00 AM, Junio C Hamano wrote: * "git branch" and "git checkout -b" are now forbidden from creating a branch whose name is "HEAD". "git branch" already forbid a branch named "HEAD", didn't it? I thought we just made "git checkout -b" to reject "HEAD" as a valid branch name, recently. -- Kaartic
Re: Error in `git': free(): invalid pointer (was Re: [PATCH] sequencer: improve config handling)
On Friday 22 December 2017 05:19 PM, Johannes Schindelin wrote: Hi Kaartic, I think I didn't mention I've set `commit.gpgsign` to `true` for that repo, did I? Hah! I had troubles to associate the correct line in my versions of Git's source code (the line numbers alone are only reliable if you also have a commit from which the Git binaries were built), Should have mentioned that, sorry. but I did this free() at (https://github.com/git/git/blob/cd54ea2b18/sequencer.c#L1975: if (read_oneliner(, rebase_path_gpg_sign_opt(), 1)) { if (!starts_with(buf.buf, "-S")) strbuf_reset(); else { free(opts->gpg_sign); ^ opts->gpg_sign = xstrdup(buf.buf + 2); } strbuf_reset(); } Seems you got the right one, regardless. :) The culprit is that we have these really unclear ownership rules, where it is not at all clear who is responsible for releasing allocated memory: caller or callee? (Hannes would not rightfully point out that this would be a non-issue if we would switch to C++). In C, the common pattern is to use `const char *` for users that are *not* supposed to take ownership, and `char *` for users that are supposed to take custody. And in this instance, `gpg_sign` should be owned by `struct replay_opts` because of this (https://github.com/git/git/blob/cd54ea2b18/sequencer.h#L38): char *gpg_sign; Technically, using `char *` (instead of `const char *`) does not say "you're now responsible for de-allocating this memory", of course, but in practice it is often used like this (and the signature of `free(void *)` certainly encourages that type of interpreting the `const` qualifier). Nice explanation. I spent a little quality time with the code and came up with a patch that I will send out in a moment. That relieves Philip of the burden, I guess. :) By the way, Kaartic, thank you so much for focusing on correctness before focusing on style issues. I know it is harder to review patches for correctness than it is to concentrate on style issues, but in my mind it is not only much more work, but also a lot more valuable. Though it's good to hear these words and I do like to focus on correctness, there wasn't much I did to focus on correctness in this case ;-) It was you actually, after seeing such a clear explanation!. I just used Git built from 'next' for my personal use and encountered the issue I stated in the start of this sub-thread. -- Kaartic
Re: Error in `git': free(): invalid pointer (was Re: [PATCH] sequencer: improve config handling)
On Thu, 2017-12-21 at 16:53 +, phillip.w...@talktalk.net wrote: > Hm, There is a problem with sequencer_remove_state() which does > > free(opts->gpg_sign) > > however unless a gpg key was given on the commandline, opts->gpg is > initialized to "" which is statically allocated. > > This untested diff should fix that, It did seem to. I don't get that error any more. > but I'm not sure if you're problem > is related to it As the fix you suggested did work, I think the problem is related. Did you have anything else in mind when you said you're not sure whether or not it's related? > also I'm on webmail so apologies if the patch is > mangled) > No issues. The patch seems quite small :) Thanks, Kaartic
Re: Error in `git': free(): invalid pointer (was Re: [PATCH] sequencer: improve config handling)
On Thu, 2017-12-21 at 15:06 +0100, Johannes Schindelin wrote: > Hi Kaartic, > I fear that the strace does not cover the `git-rebase` process nor the > `git-rebase--helper` process (which must have been part of your run, as > the commit affected only that part of the rebase operation). > Yep. It striked me only later that the entry point of "git rebase" is essentially a script. > If you have valgrind installed, you may want to give it a try. Since > git-rebase is (still, much to my dislike) a Unix shell script, you will > have to work quite hard to get it to valgrind correctly. > You're right about that it's quite hard to get to the right point of the issue when scripts are involved. > This is how I typically do it: > Then, I look for that call (I imagine it is the `exec git rebase--helper > ${force_rebase:+--no-ff} --continue` line), then copy-edit it and guard it > by an environment variable: > > test -z "$" || { > valgrind git rebase--helper ${force_rebase:+--no-ff} \ > --continue > exit > } > exec git rebase--helper ${force_rebase:+--no-ff} --continue > > After that, you can run the same command, and trigger the new code path by > that environment variable: > > =1 /mnt/Source/Git/git rebase -i HEAD~10 > > That way, you can keep the rest of your Git calls be unaffected by the > patch. > Thanks for detailing the way to get the right information for this issue. That was intriguing. > BTW from your invocation, I imagine that you wanted to actually run your > Git just-built-from-source, in-place. But for that, you would need to pass > the --exec-path option, too, like so: > > =1 /mnt/Source/Git/git --exec-path=/mnt/Source/Git \ > rebase -i HEAD~10 > > That way, you could edit the git-rebase--interactive file that is *not* > installed, and avoid affecting other Git operations (you would also not > need to guard the new code path behind a conditional). > Makes sense. (I had actually used incorrect directories in my previous mail as I did part of it manually (big mistake!) so the trace itself might not have much sense in some parts, sorry) Speaking of trace, (thanks to Dscho!) the one I got using 'valgrind' (with `--leak-check=full` option) can be found below. I've kept only relevant parts of it to avoid unwanted noise of `set -x`. Let me know if that's needed too. ... + git diff-files --quiet --ignore-submodules + git diff-index --cached --quiet --ignore-submodules HEAD -- + test 0 = 1 + test -z 1 + valgrind --leak-check=full git rebase--helper --continue ==10384== Memcheck, a memory error detector ==10384== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al. ==10384== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright info ==10384== Command: git rebase--helper --continue ==10384== ==10384== Invalid free() / delete / delete[] / realloc() ==10384==at 0x4C2CDDB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==10384==by 0x24E3F8: read_populate_opts (sequencer.c:1964) ==10384==by 0x24E3F8: sequencer_continue (sequencer.c:2753) ==10384==by 0x1735DC: cmd_rebase__helper (rebase--helper.c:52) ==10384==by 0x11B847: run_builtin (git.c:346) ==10384==by 0x11B847: handle_builtin (git.c:554) ==10384==by 0x11BB05: run_argv (git.c:606) ==10384==by 0x11BB05: cmd_main (git.c:683) ==10384==by 0x11AC0B: main (common-main.c:43) ==10384== Address 0x2a898a is in a r-x mapped file /mnt/Source/Git/git-next/git segment ==10384== Successfully rebased and updated refs/heads/public. ==10384== ==10384== HEAP SUMMARY: ==10384== in use at exit: 680,882 bytes in 332 blocks ==10384== total heap usage: 717 allocs, 386 frees, 1,709,293 bytes allocated ==10384== ==10384== 2,053 (2,048 direct, 5 indirect) bytes in 1 blocks are definitely lost in loss record 75 of 83 ==10384==at 0x4C2BADF: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==10384==by 0x4C2DE5F: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==10384==by 0x27E0FE: xrealloc (wrapper.c:138) ==10384==by 0x2072A3: add_object_array_with_path (object.c:319) ==10384==by 0x23D745: add_pending_object_with_path (revision.c:163) ==10384==by 0x24030E: add_pending_object_with_mode (revision.c:170) ==10384==by 0x24030E: add_pending_object (revision.c:176) ==10384==by 0x24030E: add_head_to_pending (revision.c:188) ==10384==by 0x280EFA: has_uncommitted_changes.part.17 (wt-status.c:2288) ==10384==by 0x24DF88: commit_staged_changes (sequencer.c:2705) ==10384==by 0x24DF88: sequencer_continue (sequencer.c:2749) ==10384==by 0x1735DC: cmd_rebase__helper (rebase--helper.c:52) ==10384==by 0x11B847: run_builtin (git.c:346) ==10384==by 0x11B847: handle_builtin (git.c:554) ==10384==by 0x11BB05: run_argv (git.c:606) ==10384==by 0x11BB05: cmd_main (git.c:683) ==10384==by 0x11AC0B: main
Re: Error in `git': free(): invalid pointer (was Re: [PATCH] sequencer: improve config handling)
On Thursday 21 December 2017 02:09 AM, phillip.w...@talktalk.net wrote: > > > Hi Kaartic > > I'm replying off list as I've only got access to webmail at the > moment. No issues brought the CCs including the list to ensure we don't miss things. > Are you able to do a backtrace with debugging symbols > please, I'm finding it hard to glean anything from the backtrace > here. That's confusing, I already had the following in my `config.mak` while compiling, DEVELOPER=1 DEVELOPERS=1 CFLAGS += -g -O2 CFLAGS += -DFLEX_ARRAY=2048 NO_GETTEXT=1 prefix=/home/$(USER)/.local #CFLAGS += -Wno-unused-value As you can see the `-g` flag is there in CFLAGS. If this output doesn't give much sense could you be specific about what output you want i.e., by mentioning the command, showing a sample etc., I'm not sure how to go about debugging with "git rebase" as it's a script. > Also does it definitely bisect to this patch or the merge of > this branch? > "git bisect" speaking, 28d6daed4f119940ace31e523b3b272d3d153d04 is the first bad commit commit 28d6daed4f119940ace31e523b3b272d3d153d04 Author: Phillip WoodDate: Wed Dec 13 11:46:21 2017 + sequencer: improve config handling The previous config handling relied on global variables, called git_default_config() even when the key had already been handled by git_sequencer_config() and did not initialize the diff configuration variables. Improve this by: i) loading the default values for message cleanup and gpg signing of commits into struct replay_opts; ii) restructuring the code to return immediately once a key is handled; and iii) calling git_diff_basic_config(). Note that unfortunately it is not possible to return early if the key is handled by git_gpg_config() as it does not indicate to the caller if the key has been handled or not. The sequencer should probably have been calling git_diff_basic_config() before as it creates a patch when there are conflicts. The shell version uses 'diff-tree' to create the patch so calling git_diff_basic_config() should match that. Although 'git commit' calls git_diff_ui_config() I don't think the output of print_commit_summary() is affected by anything that is loaded by that as print_commit_summary() always turns on rename detection so would ignore the value in the user's configuration anyway. The other values loaded by git_diff_ui_config() are about the formatting of patches so are not relevant to print_commit_summary(). Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano :04 04 bbd487cc285ae12cefb97a9661de4ff27d182727 9460ac528bf68f331088412f62e5d6a39305854b M builtin :100644 100644 99452a0e89777999418c116386f924be396ed04a 7051b20b76292e3b539875f0dc38108a734d2164 M sequencer.c :100644 100644 77cb174b2aaf3972ebb9e6ec379252be96dedd3d 3a5072c2ab9088c237b83d92deae3c801289e543 M sequencer.h FWIW, I started the bisection with `v2.11.0` as good (because "git rebase -i HEAD~10 worked there) and the tip of `next` (cfbfd45ee (Sync with master, 2017-12-19)) as bad. Here's the bisect log for a shorter range of bisection (which was possible after I discovered the bad commit in the previous bisect) git bisect start # bad: [cfbfd45ee6e49007fdeb8904064ba98f65e0] Sync with master git bisect bad cfbfd45ee6e49007fdeb8904064ba98f65e0 # good: [a4212f7ebd7158a1e237d776fb4bbd7a295d0bda] Merge branch 'pw/sequencer-in-process-commit' into next git bisect good a4212f7ebd7158a1e237d776fb4bbd7a295d0bda # good: [bdae4af87053490adad2dc9fb184d6d050d46a4c] Merge branch 'sg/setup-doc-update' git bisect good bdae4af87053490adad2dc9fb184d6d050d46a4c # bad: [6d12e08217360cfc042ec484129771f73ad7b97f] Merge branch 'rs/strbuf-read-once-reset-length' into next git bisect bad 6d12e08217360cfc042ec484129771f73ad7b97f # bad: [1b791d2503742b060f91b8159b85c06335634bd8] Merge branch 'ab/simplify-perl-makefile' into next git bisect bad 1b791d2503742b060f91b8159b85c06335634bd8 # bad: [abfed699db9f0f0e6f03f5ee3e9d137944ba4216] Merge branch 'sb/clone-recursive-submodule-doc' into next git bisect bad abfed699db9f0f0e6f03f5ee3e9d137944ba4216 # bad: [ec4d2b9c843b8b338ffa8906eea70095b3098c1e] Merge branch 'pw/sequencer-in-process-commit' into next git bisect bad ec4d2b9c843b8b338ffa8906eea70095b3098c1e # good: [5279b80103bde3ec5d6108fa8f43de2017668039] Merge branch 'tb/check-crlf-for-safe-crlf' into next git bisect good 5279b80103bde3ec5d6108fa8f43de2017668039 # bad: [28d6daed4f119940ace31e523b3b272d3d153d04] sequencer: improve config handling git bisect bad 28d6daed4f119940ace31e523b3b272d3d153d04 # first bad commit: [28d6daed4f119940ace31e523b3b272d3d153d04] sequencer: improve config handling -- Kaartic
Error in `git': free(): invalid pointer (was Re: [PATCH] sequencer: improve config handling)
I recently encountered that error when trying to do an interactive rebase after using filter-branch to remove a file completely in a repository. I bisected this issue which pointed at this patch. I'm not sure how it is related as I'm not too familiar with the sequencer code. I could help in case any specific information is needed. As a first step, I've posted the output of "strace /mnt/Source//Git/git rebase -i HEAD~10" below. -- 8< -- execve("/mnt/Source//Git/git-next/git", ["/mnt/Source//Git/git-next/git", "rebase", "-i", "HEAD~10"], [/* 62 vars */]) = 0 brk(NULL) = 0x55a494d3 access("/etc/ld.so.nohwcap", F_OK) = -1 ENOENT (No such file or directory) mmap(NULL, 12288, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f25bd94 access("/etc/ld.so.preload", R_OK) = -1 ENOENT (No such file or directory) open("/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3 fstat(3, {st_mode=S_IFREG|0644, st_size=114620, ...}) = 0 mmap(NULL, 114620, PROT_READ, MAP_PRIVATE, 3, 0) = 0x7f25bd924000 close(3)= 0 access("/etc/ld.so.nohwcap", F_OK) = -1 ENOENT (No such file or directory) open("/lib/x86_64-linux-gnu/libz.so.1", O_RDONLY|O_CLOEXEC) = 3 read(3, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\300!\0\0\0\0\0\0"..., 832) = 832 fstat(3, {st_mode=S_IFREG|0644, st_size=105088, ...}) = 0 mmap(NULL, 2200072, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x7f25bd506000 mprotect(0x7f25bd51f000, 2093056, PROT_NONE) = 0 mmap(0x7f25bd71e000, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x18000) = 0x7f25bd71e000 close(3)= 0 access("/etc/ld.so.nohwcap", F_OK) = -1 ENOENT (No such file or directory) open("/lib/x86_64-linux-gnu/libpthread.so.0", O_RDONLY|O_CLOEXEC) = 3 read(3, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0Pa\0\0\0\0\0\0"..., 832) = 832 fstat(3, {st_mode=S_IFREG|0755, st_size=135440, ...}) = 0 mmap(NULL, 2212936, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x7f25bd2e9000 mprotect(0x7f25bd301000, 2093056, PROT_NONE) = 0 mmap(0x7f25bd50, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x17000) = 0x7f25bd50 mmap(0x7f25bd502000, 13384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x7f25bd502000 close(3)= 0 access("/etc/ld.so.nohwcap", F_OK) = -1 ENOENT (No such file or directory) open("/lib/x86_64-linux-gnu/librt.so.1", O_RDONLY|O_CLOEXEC) = 3 read(3, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\340 \0\0\0\0\0\0"..., 832) = 832 fstat(3, {st_mode=S_IFREG|0644, st_size=31744, ...}) = 0 mmap(NULL, 2128832, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x7f25bd0e1000 mprotect(0x7f25bd0e8000, 2093056, PROT_NONE) = 0 mmap(0x7f25bd2e7000, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x6000) = 0x7f25bd2e7000 close(3)= 0 access("/etc/ld.so.nohwcap", F_OK) = -1 ENOENT (No such file or directory) open("/lib/x86_64-linux-gnu/libc.so.6", O_RDONLY|O_CLOEXEC) = 3 read(3, "\177ELF\2\1\1\3\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\0\4\2\0\0\0\0\0"..., 832) = 832 fstat(3, {st_mode=S_IFREG|0755, st_size=1689360, ...}) = 0 mmap(NULL, 3795296, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x7f25bcd42000 mprotect(0x7f25bced7000, 2097152, PROT_NONE) = 0 mmap(0x7f25bd0d7000, 24576, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x195000) = 0x7f25bd0d7000 mmap(0x7f25bd0dd000, 14688, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x7f25bd0dd000 close(3)= 0 mmap(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f25bd922000 arch_prctl(ARCH_SET_FS, 0x7f25bd922e80) = 0 mprotect(0x7f25bd0d7000, 16384, PROT_READ) = 0 mprotect(0x7f25bd50, 4096, PROT_READ) = 0 mprotect(0x7f25bd2e7000, 4096, PROT_READ) = 0 mprotect(0x7f25bd71e000, 4096, PROT_READ) = 0 mprotect(0x55a49424b000, 12288, PROT_READ) = 0 mprotect(0x7f25bd943000, 4096, PROT_READ) = 0 munmap(0x7f25bd924000, 114620) = 0 set_tid_address(0x7f25bd923150) = 9667 set_robust_list(0x7f25bd923160, 24) = 0 rt_sigaction(SIGRTMIN, {sa_handler=0x7f25bd2eebd0, sa_mask=[], sa_flags=SA_RESTORER|SA_SIGINFO, sa_restorer=0x7f25bd2fa0c0}, NULL, 8) = 0 rt_sigaction(SIGRT_1, {sa_handler=0x7f25bd2eec60, sa_mask=[], sa_flags=SA_RESTORER|SA_RESTART|SA_SIGINFO, sa_restorer=0x7f25bd2fa0c0}, NULL, 8) = 0 rt_sigprocmask(SIG_UNBLOCK, [RTMIN RT_1], NULL, 8) = 0 getrlimit(RLIMIT_STACK, {rlim_cur=8192*1024, rlim_max=RLIM64_INFINITY}) = 0 open("/dev/null", O_RDWR) = 3 close(3)= 0 rt_sigprocmask(SIG_UNBLOCK, [PIPE], NULL, 8) = 0 rt_sigaction(SIGPIPE, {sa_handler=SIG_DFL, sa_mask=[PIPE], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7f25bcd75060}, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=0}, 8) = 0 brk(NULL)
Re: What's cooking in git.git (Dec 2017, #04; Tue, 19)
On Wednesday 20 December 2017 03:30 AM, Junio C Hamano wrote: * pw/sequencer-in-process-commit (2017-12-13) 10 commits (merged to 'next' on 2017-12-13 at ec4d2b9c84) ... The sequencer infrastructure is shared across "git cherry-pick", "git rebase -i", etc., and has always spawned "git commit" when it needs to create a commit. It has been taught to do so internally, when able, by reusing the codepath "git commit" itself uses, which gives performance boost for a few tens of percents in some sample scenarios. Will merge to 'master'. I just encountered a small issue with 'rebase -i'. My bisection pointed at, 28d6daed4 (sequencer: improve config handling, 2017-12-13) So, I guess it would be better if you could delay merging it to 'master'. I'm not sure what how the commit and the patch are related but lets discuss that in the patch thread. -- Kaartic
[PATCH v3] imap-send: URI encode server folder
From: Nicolas Morey-Chaisemartin <nmoreychaisemar...@suse.com> When trying to send a patch using 'imap-send' with 'curl' and the following configuration: [imap] folder = "[Gmail]/Drafts" host = imaps://imap.gmail.com port = 993 sslverify = false results in the following error, curl_easy_perform() failed: URL using bad/illegal format or missing URL This is a consequence of not URI-encoding the folder portion of the URL which contains characters such as '[' which are not allowed in a URI. According to RFC3986, these characters should be URI-encoded. So, URI-encode the folder before adding it to the URI to ensure it doesn't contain characters that aren't allowed in a URI. Reported-by: Doron Behar <doron.be...@gmail.com> Signed-off-by: Nicolas Morey-Chaisemartin <nmoreychaisemar...@suse.com> Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- Changes in v3: - updated commit message as suggested by Eric (convert past tense to present tense) and added a few tweaks to it that striked me Eric Sunshine <sunsh...@sunshineco.com> writes: > Thanks for taking up the slack. You're welcome. It was easier than waiting for this patch to be updated so it could get into 'pu' ;-) imap-send.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/imap-send.c b/imap-send.c index 54e6a80fd..36c7c1b4f 100644 --- a/imap-send.c +++ b/imap-send.c @@ -1412,6 +1412,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred) { CURL *curl; struct strbuf path = STRBUF_INIT; + char *uri_encoded_folder; if (curl_global_init(CURL_GLOBAL_ALL) != CURLE_OK) die("curl_global_init failed"); @@ -1429,7 +1430,12 @@ static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred) strbuf_addstr(, server.host); if (!path.len || path.buf[path.len - 1] != '/') strbuf_addch(, '/'); - strbuf_addstr(, server.folder); + + uri_encoded_folder = curl_easy_escape(curl, server.folder, 0); + if (!uri_encoded_folder) + die("failed to encode server folder"); + strbuf_addstr(, uri_encoded_folder); + curl_free(uri_encoded_folder); curl_easy_setopt(curl, CURLOPT_URL, path.buf); strbuf_release(); -- 2.15.1.620.gb9897f467
Re: 'git format-patch --summary' seems to be turning off the stat
On Tuesday 19 December 2017 12:07 AM, Junio C Hamano wrote: Kaartic Sivaraam <kaartic.sivar...@gmail.com> writes: Note: I do see that "--summary" is a diff-option but does that mean we should't be printing stat information in the patch when the user didn't mention "--no-stat"? Yeah, "git format-patch --summary --stat" does bring back the stat. --- Kaartic The old design decision shared across "log" family of commands [*1*] wrt to these "how and what are shown" options is that they started without any default set of options and the user gave all options (if they want both summary and stat, they gave them from the command line); over time they gained a set of built-in default options that take effect only when the user gives no option. So, yes, giving "--summary" will turn off the built-in ones that are "--summary --stat" (IIRC). Is it worth documenting that built-in defaults are turned off when user gives other options? Not sure where, though? -- Kaartic
[PATCH v2] imap-send: URI encode server folder
From: Nicolas Morey-ChaisemartinWhen trying to send a patch using 'imap-send' with 'curl' and the following configuration: [imap] folder = "[Gmail]/Drafts" host = imaps://imap.gmail.com port = 993 sslverify = false resulted in the following error, curl_easy_perform() failed: URL using bad/illegal format or missing URL That was a consequence of the not URI encoding the folder portion of the URL which contained characters such as '[' which are not allowed in a URI. According to RFC3986, these characters should be "URI encoded". So, URI encode the folder portion of the URL to ensure it doesn't contain characters that aren't allowed in a URI. Reported-by: Doron Behar Signed-off-by: Nicolas Morey-Chaisemartin --- I came across the same issue that lead to this patch recently and found that this patch didn't make it in. So, I thought I could help out and hence this v2. Eric Sunshine writes: > For someone reading this commit message in the future -- someone who > didn't follow the email thread which led to this patch -- "this fixes" > doesn't say much about the actual problem being addressed. Can you > expand the commit message a bit to make it more self-contained? At > minimum, perhaps show the error message you were experiencing, and > cite (as Daniel pointed out) RFC 3986 and the bit about a "legal" URL > not containing brackets. I guess I covered this part. > Also, a natural question which pops into the head of someone reading > this patch is whether other parts of the URL (host, user, etc.) also > need to be handled similarly. It's possible that you audited the code > and determined that they are handled fine already, but the reader of > the commit message is unable to infer that. Consequently, it might be > nice to have a sentence about that, as well ("other parts of the URL > are already encoded, thus are fine" or "other parts of the URL are not > subject to this problem because ..."). I'm not sure about this one. I guess the host and user don't need encoding as I suspect they wouldn't contain characters that aren't allowed. I might be wrong, though. Let me know if I'm missing something. Thanks, Kaartic imap-send.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/imap-send.c b/imap-send.c index 54e6a80fd..36c7c1b4f 100644 --- a/imap-send.c +++ b/imap-send.c @@ -1412,6 +1412,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred) { CURL *curl; struct strbuf path = STRBUF_INIT; + char *uri_encoded_folder; if (curl_global_init(CURL_GLOBAL_ALL) != CURLE_OK) die("curl_global_init failed"); @@ -1429,7 +1430,12 @@ static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred) strbuf_addstr(, server.host); if (!path.len || path.buf[path.len - 1] != '/') strbuf_addch(, '/'); - strbuf_addstr(, server.folder); + + uri_encoded_folder = curl_easy_escape(curl, server.folder, 0); + if (!uri_encoded_folder) + die("failed to encode server folder"); + strbuf_addstr(, uri_encoded_folder); + curl_free(uri_encoded_folder); curl_easy_setopt(curl, CURLOPT_URL, path.buf); strbuf_release(); -- 2.15.1.620.gb9897f467
'git format-patch --summary' seems to be turning off the stat
The documentation for "--summary" option in the format-patch Doc states, --summary Output a condensed summary of extended header information such as creations, renames and mode changes. It doesn't state anything about turning of the stat. Why does the stat get turned off when "--summary" option is used along with "git format-patch" ? Note: I do see that "--summary" is a diff-option but does that mean we should't be printing stat information in the patch when the user didn't mention "--no-stat"? Yeah, "git format-patch --summary --stat" does bring back the stat. --- Kaartic
[PATCH v5 2/3] rebase: distinguish user input by quoting it
Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> Signed-off-by: Junio C Hamano <gits...@pobox.com> Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- git-rebase.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/git-rebase.sh b/git-rebase.sh index a299bcc52..0f379ba2b 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -477,7 +477,7 @@ then ;; esac upstream=$(peel_committish "${upstream_name}") || - die "$(eval_gettext "invalid upstream \$upstream_name")" + die "$(eval_gettext "invalid upstream '\$upstream_name'")" upstream_arg="$upstream_name" else if test -z "$onto" @@ -539,7 +539,7 @@ case "$#" in head_name="detached HEAD" else - die "$(eval_gettext "fatal: no such branch/commit: \$branch_name")" + die "$(eval_gettext "fatal: no such branch/commit '\$branch_name'")" fi ;; 0) -- 2.15.0.531.g2ccb3012c
[PATCH v5 3/3] rebase: rebasing can also be done when HEAD is detached
Attempting to rebase when the HEAD is detached and is already up to date with upstream (so there's nothing to do), the following message is shown Current branch HEAD is up to date. which is clearly wrong as HEAD is not a branch. Handle the special case of HEAD correctly to give a more precise error message. Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> Signed-off-by: Junio C Hamano <gits...@pobox.com> Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com> --- git-rebase.sh | 16 ++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/git-rebase.sh b/git-rebase.sh index 0f379ba2b..fd72a35c6 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -601,11 +601,23 @@ then test -z "$switch_to" || GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to" \ git checkout -q "$switch_to" -- - say "$(eval_gettext "Current branch \$branch_name is up to date.")" + if test "$branch_name" = "HEAD" && +! git symbolic-ref -q HEAD + then + say "$(eval_gettext "HEAD is up to date.")" + else + say "$(eval_gettext "Current branch \$branch_name is up to date.")" + fi finish_rebase exit 0 else - say "$(eval_gettext "Current branch \$branch_name is up to date, rebase forced.")" + if test "$branch_name" = "HEAD" && +! git symbolic-ref -q HEAD + then + say "$(eval_gettext "HEAD is up to date, rebase forced.")" + else + say "$(eval_gettext "Current branch \$branch_name is up to date, rebase forced.")" + fi fi fi -- 2.15.0.531.g2ccb3012c