From: Clement Mabileau Date: Thu, 23 Mar 2023 15:51:22 GMT Subject: Re: [PATCH v2] branch: improve error log on branch not found by checking remotes refs Message-ID: In-Reply-To: On 22/03/2023 23:25, Junio C Hamano wrote: > "ClementMabileau via GitGitGadget" writes: > >> From: ctmbl > > This "name
" should match what is on "Signed-off-by:". I > offhand do not know where GitGitGadget gets the information, but I > suspect the original commit object you created records ctmbl as the > author name, not "Clement Mabileau"? I can fix it for this single > time, but if you plan to contribute to the project in an ongoing > basis, you may want to fix your .git/config (or $HOME/.gitconfig) > with a "[user] name = ..." entry. Well maybe it got it from the original commit or from my GitHub username, I can't know for sure. But sorry for that :/ , I'll make sure it's fixed it for potential future contribution! > If the user said "git branch -d frotz", we would have 'frotz' > there, right? Then we apply FMT_REMOTES (Yuck, now I have to go > scroll up, see that it is set to "refs/remotes/%s", and hope or > verify that its value hasn't been changed in the meantime---in > short, don't introduce that variable. A macro might be OK, but I do > not see much point here) and we get "refs/remotes/frotz" back. > > So, we check "refs/remotes/frotz" here ... > >> [...] > > ... but why should we? If your workflow interacts with the original > repository you cloned from, you would have remote-tracking branches > like "refs/remotes/origin/frotz" and it may be plausible that you > meant to remove with "git branch -d frotz" the remote-tracking > branch "refs/remotes/origin/frotz". But the new code makes no > effort to figure out the name of the remote (e.g. 'origin') here, > and I am not sure what value it adds to check and try to tell the > user about "refs/remotes/frotz". Or are we assuming that the user > would say "git branch -d origin/frotz" in such a case? Before fixing anything about the code maybe I should first address your last point which is the interest of the patch in the first place (and I should have started with that...). A few months earlier, for the first time, I had to delete a remote ref (because of a fork I fetched but no longer wanted: maybe a designed solution exists but I'm not aware of it). However, despite being used to git I had a hard time figuring out how to do it, I tried different things, one was `git branch -d origin/` (I recently discovered that it was written in `git branch --help` but I didn't find it at the time). Even googling it proved difficult (because of a poor keyword choice I must confess), most results was dealing with deleting remote branches, such as `git push remote :branch`. In the end, I finally understood that I needed that `--remote` flag and really regretted that there wasn't an hint message to head me towards the solution when I was getting close to it. Now I hope you'll understand why I suggested this patch. Maybe I'm the only one that ended up in this situation, in this case I'd understand that you would no longer be interested in the patch! However if you still are, I'll be happy to make the modification you asked for. >> + if (virtual_target) >> + error(_(MISSING_BRANCH_HINT_MSG), bname.buf); >> + else >> + error(_(MISSING_BRANCH_ERROR_MSG), bname.buf); >> + } > > Are you leaking virtual_target here? Yeah probably, I'll fix it along with the other. Thanks for reviewing this!