On Fri, Apr 29, 2016 at 9:50 AM, SZEDER Gábor <szeder@xxxxxxxxxx> wrote: >> Executing `git-rev-parse` with `--git-common-dir`, `--git-path <path>`, >> or `--shared-index-path` from the root of the main worktree results in >> a relative path to the git dir. >> >> When executed from a subdirectory of the main tree, however, it incorrectly >> returns a path which starts 'sub/path/.git'. > > This is not completely true, because '--git-path ...' returns a > relative path starting with '.git': > > $ git -C t/ rev-parse --git-dir --git-path objects --git-common-dir > /home/szeder/src/git/.git > .git/objects > t/.git > > It's still wrong, of course. I'll try to reword this to make it indicate that the value isn't always incorrect. > >> Change this to return the >> proper relative path to the git directory. > > I think returning absolute paths would be better. It is consistent > with the already properly working '--git-dir' option, which returns an > absolute path in this case. Furthermore, both '--git-path ...' and > '--git-common-dir' already return absolute paths when run from a > subdirectory of the .git directory: > > $ git -C .git/refs rev-parse --git-dir --git-path objects --git-common-dir > /home/szeder/src/git/.git > /home/szeder/src/git/.git/objects > /home/szeder/src/git/.git > I agree with this, but at one point Junio suggested that it should return the relative path[1], so I went with that. Maybe I could RFC a separate patch that changes all of these to absolute. >> Signed-off-by: Michael Rappazzo <rappazzo@xxxxxxxxx> >> --- >> builtin/rev-parse.c | 19 ++++++++++++++----- >> 1 file changed, 14 insertions(+), 5 deletions(-) > > This patch doesn't add any new tests, while subsequent patches of the > series do nothing but add more tests. Splitting up your changes this > way doesn't add any value, it only increases the number of commits. I > think either: > > - all those new tests could be added with this patch, or > > - if you want to add the test separately, then add them before > this patch and mark them with 'test_expect_failure' to clearly > demonstrate what the series is about to fix, and flip them to > 'test_expect_success' in this patch. > > - An alternative way to split this series, following the "Make > separate commits for logically separate changes" guideline, would > be to fix and test these options in separate patches, i.e. fix and > test '--git-path ...' in one patch, then fix and test > '--git-common-dir' in the next, ... > > Thanks, have a re-roll ready to go based on you input. I went with option 2 - add tests with expect failure and fix them with the next. [1] http://article.gmane.org/gmane.comp.version-control.git/290061 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html